update.go: refactor calculatePostConfigChangeAction() #3022

mkenigs · 2022-03-16T13:14:56Z

Currently, post config change actions are calculated as follows:
1) Reboot if there's a force file
2) Reboot if any MachineConfig fields change except for passwd
3) Calculate an action based on file changes

Layered updates will need to calculate post config change actions solely
based on files, as they will be diffing two images. 1) and 3) can be
consolidated into a single function that is shared between layered and
legacy updates. All MachineConfig changes (2) will result in file
changes on disk, which will cause a reboot if actions are calculated as
normal. This is correct for any change except for changes to passwd,
which currently do not require any post config change action. In order
to preserve this behavior, add a reboot file exception for ssh keys,
which is the only field the MCO supports modifying using passwd.

Add some test cases to check that changes to ssh keys result in
postConfigChangeActionNone when appropriate

kikisdeliveryservice

a few questions/comments

pkg/daemon/update.go

pkg/daemon/update_test.go

Currently, post config change actions are calculated as follows: 1) Reboot if there's a force file 2) Reboot if any MachineConfig fields change except for passwd 3) Calculate an action based on file changes Layered updates will need to calculate post config change actions solely based on files, as they will be diffing two images. 1) and 3) can be consolidated into a single function that is shared between layered and legacy updates. All MachineConfig changes (2) will result in file changes on disk, which will cause a reboot if actions are calculated as normal. This is correct for any change except for changes to passwd, which currently do not require any post config change action. In order to preserve this behavior, add a reboot file exception for ssh keys, which is the only field the MCO supports modifying using passwd.

Add some test cases to check that changes to ssh keys result in postConfigChangeActionNone when appropriate

kikisdeliveryservice · 2022-03-24T00:45:00Z

some aws IAM issues (across PRs) reported in 4-dev

/test e2e-aws-upgrade
/test e2e-aws

kikisdeliveryservice

Good job, Matthew. It's a nice refactor the function flow makes sense to me. And we got to learn about Variant IDs 😆

mkenigs · 2022-03-24T12:14:25Z

Good job, Matthew. It's a nice refactor the function flow makes sense to me. And we got to learn about Variant IDs laughing

Thanks, that's nice to hear :) Thanks for the review

kikisdeliveryservice · 2022-03-29T19:04:57Z

this needs another review and lgtm

@jkyros @cheesesashimi can you PTAL

jkyros

This makes sense to me and lets us use calculatePostConfigChangeActionFromFiles for layering without breaking existing.
/lgtm

jkyros · 2022-03-29T19:47:58Z

pkg/daemon/update.go

+	// If a machine-config-daemon-force file is present, it means the user wants to
+	// move to desired state without additional validation. We will reboot the node in
+	// this case regardless of what MachineConfig diff is.
+	if _, err := os.Stat(constants.MachineConfigDaemonForceFile); err == nil {


This feels maybe little weird having the forcefile check inside calculatePostConfigChangeActionFromFiles but its' no worse than what we had. Like, we factored out the other criteria so this function would suppoooosedly be making a decision just on this list of files provided...but then we also sneak out to the host's disk to check for a file (which means this function can't run as a 'hypothetical' against a list of files, it would need access to a host disk).

From a "find the largest unit of code that both layered and non-layered can use" standpoint this is good.

Cool for now, not worth holding this up for that, just something to think about for later.

Yeah that's a good point

openshift-ci · 2022-03-29T19:49:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkyros, kikisdeliveryservice, mkenigs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kikisdeliveryservice]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2022-03-29T20:52:29Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-03-29T22:58:32Z

@mkenigs: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-single-node	`657f3d0`	link	false	`/test e2e-gcp-single-node`
ci/prow/e2e-gcp-op-single-node	`657f3d0`	link	false	`/test e2e-gcp-op-single-node`
ci/prow/e2e-aws-upgrade	`657f3d0`	link	false	`/test e2e-aws-upgrade`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2022-03-30T04:38:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-30T05:14:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-30T05:38:56Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

mkenigs · 2022-03-30T12:10:29Z

@kikisdeliveryservice do we want to cherry pick this?

openshift-ci bot requested review from cheesesashimi and jkyros March 16, 2022 13:15

mkenigs mentioned this pull request Mar 16, 2022

Factor out functionality from update() that will be needed for layering #2987

Merged

kikisdeliveryservice reviewed Mar 16, 2022

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

pkg/daemon/update.go Outdated Show resolved Hide resolved

pkg/daemon/update.go Outdated Show resolved Hide resolved

pkg/daemon/update.go Outdated Show resolved Hide resolved

pkg/daemon/update_test.go Outdated Show resolved Hide resolved

mkenigs force-pushed the refactor-calculatePostConfigChangeAction branch from fa308f3 to c4a929b Compare March 23, 2022 19:22

test calculatePostConfigChangeAction for ssh keys

657f3d0

Add some test cases to check that changes to ssh keys result in postConfigChangeActionNone when appropriate

mkenigs force-pushed the refactor-calculatePostConfigChangeAction branch from c4a929b to 657f3d0 Compare March 23, 2022 19:41

kikisdeliveryservice approved these changes Mar 24, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022

jkyros approved these changes Mar 29, 2022

View reviewed changes

openshift-ci bot assigned jkyros Mar 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

openshift-merge-robot merged commit f856838 into openshift:layering Mar 30, 2022

mkenigs deleted the refactor-calculatePostConfigChangeAction branch March 30, 2022 12:04

update.go: refactor calculatePostConfigChangeAction() #3022

update.go: refactor calculatePostConfigChangeAction() #3022

Uh oh!

Conversation

mkenigs commented Mar 16, 2022

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kikisdeliveryservice commented Mar 24, 2022

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

mkenigs commented Mar 24, 2022

Uh oh!

kikisdeliveryservice commented Mar 29, 2022

Uh oh!

jkyros left a comment

Choose a reason for hiding this comment

Uh oh!

jkyros Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkenigs Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 29, 2022

Uh oh!

openshift-bot commented Mar 29, 2022

Uh oh!

openshift-ci bot commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Mar 30, 2022

Uh oh!

openshift-bot commented Mar 30, 2022

Uh oh!

openshift-bot commented Mar 30, 2022

Uh oh!

mkenigs commented Mar 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkyros Mar 29, 2022 •

edited

Loading

openshift-ci bot commented Mar 29, 2022 •

edited

Loading