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

o/registrystate: add registry hook handlers #14483

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

MiguelPires
Copy link
Contributor

Adds registry hook handlers. The change-view- hook handler checks the transaction to see if the snap has rejected the changes (this will be done with a snapctl fail <reason> command) and outputs that information nicely if so. The save-view- hook handler attempts to roll back previous save-view hooks that have already run if the hook failed. It saves the error, schedules rollback tasks and then outputs the error once those are done. The -view-changed doesn't have any code to it.

@MiguelPires MiguelPires added the registry registry related work (previously called aspects) label Sep 10, 2024
Adds a change-view hook handler that checks whether the snap has
rejected the changes and outputs that nicely.
Copy link
Member

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thanks! Some comments, but looks good

overlord/registrystate/registrymgr.go Outdated Show resolved Hide resolved
overlord/registrystate/registrymgr.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@pedronis pedronis self-requested a review September 19, 2024 07:33
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you, did a pass, some questions and comments

func (h *snapHookHandler) Before() error {
return nil
}
type SnapHookHandler struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to export this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -view-changed hooks can use it. I could duplicate it but it this seemed reasonable

overlord/registrystate/registrymgr.go Show resolved Hide resolved
overlord/registrystate/registrymgr.go Outdated Show resolved Hide resolved
@ernestl ernestl added this to the 2.67 milestone Sep 19, 2024
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

Comment on lines +356 to +357
err = s.o.Settle(5 * time.Second)
c.Assert(err, IsNil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this bit do something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just runs the manager's Ensure which is a no-op

// prevent the next registry tasks from running before the rollbacks. Once the
// last rollback task errors, these will be put on Hold by the usual mechanism
for _, halt := range t.HaltTasks() {
halt.WaitFor(last)
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the run hooks will also end up with HoldStatus ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the last rollback task surfaces the error, the tasks after it will be held I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
registry registry related work (previously called aspects)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants