Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The current sed script delete everything #225

Merged
merged 3 commits into from
Sep 16, 2022
Merged

Conversation

pizzamig
Copy link
Collaborator

@pizzamig pizzamig commented Sep 16, 2022

This easy fix, while not super robust, seems a first good patch

fix #224

This easy fix, while not super robust, seems a first good patch
@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

strange, I'll take a look as well

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

Oh my - it only deletes the first appearance. Unfortunately, the first time status is written, there is only one appearance.
The easiest fix would be to always write the status twice.

@pizzamig
Copy link
Collaborator Author

If you agree, I'll do an emergency release for conference reasons.
A more robust patch can come later

-e '/^pot\.status=/{n;bc' -e ':c' -e 'p;n;bc' -e '}' \
-e "p;n;ba" "$_status_file"
if [ "$(grep -c "^pot\.status=" "$_status_file")" -gt 1 ]; then
${SED} -i '' -e '1d' "$_status_file"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
${SED} -i '' -e '1d' "$_status_file"
${SED} -i '' -n -e ":a" \
-e '/^pot\.status=/{n;bc' -e ':c' -e 'p;n;bc' -e '}' \
-e "p;n;ba" "$_status_file"'

This (combined with your new check) should work fine. It always removes the first status line (but it doesn't figure out that there is only one, which is what your grep fixes now). >_<

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

If you agree, I'll do an emergency release for conference reasons. A more robust patch can come later

I can test the proposed patch right away, should work just fine, but I can confirm in a few minutes.

@pizzamig
Copy link
Collaborator Author

Just updated the patch to call sed only if there are multiple entries

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

So the patch seems to work. But there are more problems. I'll elaborate in a second.

@pizzamig
Copy link
Collaborator Author

Technically, we can rework it to overwrite the entire file with the last entry available (like get_status)

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

# pot start cloneme
###>  No ipv4 address found for cloneme
###>  pot cloneme is not in a state where it can be stopped
# pot show
# cat /tmp/pot_status_cloneme
pot.status=starting
# pot set-status -p cloneme -s stopping || echo "PROBLEM"
PROBLEM
# pot set-status -p cloneme -s started || echo "PROBLEM"
# pot set-status -p cloneme -s stopping || echo "PROBLEM"
# pot set-status -p cloneme -s stopped || echo "PROBLEM"

So basically, problems during start keep the pot stuck in "starting", since it can't go from "starting" directly to "stopping". Updated

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

Technically, we can rework it to overwrite the entire file with the last entry available (like get_status)

No, the patch is fine and solves the issue at hand. We have an unrelated problem in the logic (see details above).
IMHO this patch can land now and we can resolve the other issue in a separate PR.

Copy link
Collaborator

@grembo grembo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the problem described, but a separate issue persists.

@pizzamig pizzamig merged commit 10c837d into master Sep 16, 2022
@pizzamig pizzamig deleted the fast-set-status-fix branch September 16, 2022 14:35
@pizzamig
Copy link
Collaborator Author

I see your point

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

So basically "start-cleanup" calls "pot stop" and if the pot hasn't started yet, it can't stop (stopping->stopped).

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

I think the cleanest solution would be a new status like "doa" and allow to go from "doa" to stopping as well (just like from started).
Then start-cleanup could set "doa" (ignoring a potential error if the pot was already started and could not go to doa). Should I prepare a patch?

@grembo
Copy link
Collaborator

grembo commented Sep 16, 2022

Patch incoming...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unstoppable pot left over by nomad
2 participants