Support uninstall through the installation apply command #1851

carolynvs · 2022-01-06T19:07:58Z

What does this change

Allow someone to manage the entire installation lifecycle with a file, and not have to jump back to using uninstall. That causes problems when people are scripting porter calls based on what's in a repo, for example, they need to detect deleted files and treat them differently. Also if the bundle is already uninstalled, calling uninstall twice results in an error, whereas apply does the right thing.

Also as part of this I moved two fields, created and modified, that are really status and not part of the installation's desired state.

What issue does it fix

Part of getporter/operator#27

Notes for the reviewer

I'd like your feedback on the new field active. If you have ideas for a more clear name, let me know.

Also I realize that maybe this sets people up for tripping over "I made an installation file (but forget to set active: true) and now it's not installing and I don't know why". Perhaps there is a better name that we can use where defaulting to false, yields the more obvious behavior. So that if you omit this field, it still installs normally.

I could flip it to inactive: true so that you have to explicitly add that field to tell porter to delete it? But I'm not sure that people will get what inactive means... Suggestions welcome! 😅

Checklist

Unit Tests
Documentation
Schema (resource schema files)

carolynvs · 2022-01-14T23:00:44Z

/azp run porter-integration

azure-pipelines · 2022-01-14T23:00:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Allow someone to manage the entire installation lifecycle with a file, and not have to jump back to using uninstall. That causes problems when people are scripting porter calls based on what's in a repo, for example, they need to detect deleted files and treat them differently. Also if the bundle is already uninstalled, calling uninstall twice results in an error, whereas apply does the right thing. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs · 2022-01-18T20:20:26Z

/azp run porter-integration

azure-pipelines · 2022-01-18T20:20:34Z

Azure Pipelines successfully started running 1 pipeline(s).

VinozzZ · 2022-01-21T01:06:45Z

pkg/porter/reconcile_test.go

+		insync, err := p.IsInstallationInSync(p.RootContext, i, &run, upgradeOpts)
+		require.NoError(t, err)
+		assert.True(t, insync)
+		// Nothing is printed out in this case, the calling function will print "up-to-date" for us


I wonder if it's still a good idea to assert this behavior so we know when it changes in the future

Other tests check for the "up-to-date" message that is printed. We can't check for it in this unit test because the function that prints it isn't called.

Here is my sad attempt at visualizing the call tree and who prints what

ApplyInstallation() -> IsInstallationInSync() prints if it should be triggered performs extra logic and prints if the installation is up to date or what action will be executed

I see. That make sense. What about asserting that nothing is printed out, so we can confirm this expected behavior in the test?

assert.Contains(t, p.TestConfig.TestContext.GetError(), "")

When I went to add that check I discovered that other functions end up printing to stderr. I don't think it's worth asserting that nothing is printed in this case, it would just make the test more fragile.

I was checking for the existence of the "triggering" and "up-to-date" because they help give the user context into what's going on, and why things happened or didn't. But checking for the lack of printing something is something that is much easier to break when we make changes, like adding more log statements elsewhere. And elsewhere in our tests we do check for "up-to-date", so I feel like we have this code path covered pretty well already.

pkg/porter/reconcile.go

docs/content/quickstart/desired-state.md

Uninstall is now triggered when a CRD is deleted OR if spec.active=false. In either case we have the porter-agent call `porter installation apply`. This requires an unreleased build of porter with support for uninstalling a bundle when active=false See getporter/porter#1851. I have also updated our logging to use log.V(level) to make it easier to filter to the type of info. 5=trace, 4=debug, 3=system state, 2=app state, 1=info, 0=error. I borrowed this from another blog/k8s project but am having trouble finding it. I'm using constants so if we need to switch what the levels mean we can later. Similarly I have added trace statements so we can figure out what happened when things go wrong. Eventually we need to add instrumentation (see getporter#58 ). This PR has lots of general improvements to the operator that became relevant while implementing uninstall: * Use a well-known annotation to trigger reconcile again: `porter.sh/retry`. We are now adding that value as a label to the agent job so that we can retry without accidentally picking up the previous job if it's still there. * Use generation instead of resource version to detect changes. Resource version is updated anytime the entire installation is changed, while generation only updates when the spec changes or when the resource is deleted. * Populate the installation status indicating if we have scheduled an agent, it's running, succeeded or failed. Emiting events will be in a separate PR. There isn't more detailed information about how the run went or what action porter took. For simplicity, and to avoid data sync issues with Porter's database, the installation status is about the porter agent job only. * We reset the status when a new change is made to the installation (we aren't keeping run history for now). * Split out the reconcile logic into smaller unit-tested functions. These tests check that we are creating resources properly and that reconcile only makes a single change then requeues. * The integration test used to check a lot of details but since it's easier to check in unit tests, and the integration test is more for "does the whole thing work?" I've simplified the checks in the integration test. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

VinozzZ · 2022-01-25T22:46:34Z

LGTM
One nit about the commit message. Should we update it to reference uninstall instead of active in there as well?

carolynvs · 2022-01-26T15:27:02Z

@VinozzZ I'll make sure to review the commit message when I squash and merge.

carolynvs · 2022-01-26T15:30:30Z

/azp run porter-integration

azure-pipelines · 2022-01-26T15:30:39Z

Azure Pipelines successfully started running 1 pipeline(s).

Two pull requests, getporter#1864 and getporter#1851, were merged, and while it didn't cause a merge conflict, they didn't play well together and it broke the build. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

* Support uninstall, installation status Uninstall is now triggered when a CRD is deleted OR if spec.active=false. In either case we have the porter-agent call `porter installation apply`. This requires an unreleased build of porter with support for uninstalling a bundle when active=false See getporter/porter#1851. I have also updated our logging to use log.V(level) to make it easier to filter to the type of info. 5=trace, 4=debug, 3=system state, 2=app state, 1=info, 0=error. I borrowed this from another blog/k8s project but am having trouble finding it. I'm using constants so if we need to switch what the levels mean we can later. Similarly I have added trace statements so we can figure out what happened when things go wrong. Eventually we need to add instrumentation (see #58 ). This PR has lots of general improvements to the operator that became relevant while implementing uninstall: * Use a well-known annotation to trigger reconcile again: `porter.sh/retry`. We are now adding that value as a label to the agent job so that we can retry without accidentally picking up the previous job if it's still there. * Use generation instead of resource version to detect changes. Resource version is updated anytime the entire installation is changed, while generation only updates when the spec changes or when the resource is deleted. * Populate the installation status indicating if we have scheduled an agent, it's running, succeeded or failed. Emiting events will be in a separate PR. There isn't more detailed information about how the run went or what action porter took. For simplicity, and to avoid data sync issues with Porter's database, the installation status is about the porter agent job only. * We reset the status when a new change is made to the installation (we aren't keeping run history for now). * Split out the reconcile logic into smaller unit-tested functions. These tests check that we are creating resources properly and that reconcile only makes a single change then requeues. * The integration test used to check a lot of details but since it's easier to check in unit tests, and the integration test is more for "does the whole thing work?" I've simplified the checks in the integration test. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Use uninstalled instead of active * Also bump porter reference to pick up fix for local registries Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Fix godoc Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

) * Support uninstall through the installation apply command Allow someone to manage the entire installation lifecycle with a file, and not have to jump back to using uninstall. That causes problems when people are scripting porter calls based on what's in a repo, for example, they need to detect deleted files and treat them differently. Also if the bundle is already uninstalled, calling uninstall twice results in an error, whereas apply does the right thing. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Switch from active to uninstalled flag on installation Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Two pull requests, getporter#1864 and getporter#1851, were merged, and while it didn't cause a merge conflict, they didn't play well together and it broke the build. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

carolynvs force-pushed the uninstall-through-apply branch from cfbcb06 to f8747ff Compare January 6, 2022 19:28

carolynvs mentioned this pull request Jan 14, 2022

Implement uninstall getporter/operator#59

Merged

carolynvs force-pushed the uninstall-through-apply branch from f8747ff to 00d8549 Compare January 14, 2022 22:25

carolynvs force-pushed the uninstall-through-apply branch from 00d8549 to 40a7dd6 Compare January 18, 2022 20:06

carolynvs marked this pull request as ready for review January 20, 2022 21:11

carolynvs requested review from jeremyrickard and vdice as code owners January 20, 2022 21:11

carolynvs requested a review from VinozzZ January 20, 2022 21:15

VinozzZ reviewed Jan 21, 2022

View reviewed changes

pkg/porter/reconcile.go Show resolved Hide resolved

docs/content/quickstart/desired-state.md Outdated Show resolved Hide resolved

carolynvs mentioned this pull request Jan 24, 2022

Improve display of uninstalled installations #1853

Closed

Switch from active to uninstalled flag on installation

f8b83fe

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

carolynvs force-pushed the uninstall-through-apply branch from 023bb95 to f8b83fe Compare January 24, 2022 19:56

vdice approved these changes Jan 26, 2022

View reviewed changes

carolynvs merged commit e955679 into getporter:release/v1 Jan 26, 2022

carolynvs deleted the uninstall-through-apply branch January 26, 2022 16:41

carolynvs mentioned this pull request Jan 26, 2022

Fix bad merge #1872

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support uninstall through the installation apply command #1851

Support uninstall through the installation apply command #1851

carolynvs commented Jan 6, 2022 •

edited

Loading

carolynvs commented Jan 14, 2022

azure-pipelines bot commented Jan 14, 2022

carolynvs commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

VinozzZ Jan 21, 2022

carolynvs Jan 21, 2022

VinozzZ Jan 21, 2022

carolynvs Jan 25, 2022

VinozzZ commented Jan 25, 2022

carolynvs commented Jan 26, 2022

carolynvs commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

Support uninstall through the installation apply command #1851

Support uninstall through the installation apply command #1851

Conversation

carolynvs commented Jan 6, 2022 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

carolynvs commented Jan 14, 2022

azure-pipelines bot commented Jan 14, 2022

carolynvs commented Jan 18, 2022

azure-pipelines bot commented Jan 18, 2022

VinozzZ Jan 21, 2022

Choose a reason for hiding this comment

carolynvs Jan 21, 2022

Choose a reason for hiding this comment

VinozzZ Jan 21, 2022

Choose a reason for hiding this comment

carolynvs Jan 25, 2022

Choose a reason for hiding this comment

VinozzZ commented Jan 25, 2022

carolynvs commented Jan 26, 2022

carolynvs commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

carolynvs commented Jan 6, 2022 •

edited

Loading