Skip to content

Conversation

@beekhof
Copy link

@beekhof beekhof commented Jul 28, 2020

- What I did

Cleaned up the PoC code for openshift/enhancements#159, replaces PR #1626 and most of PR #1922
Prepares the way for config changes that do not require reboots to be applied.

- How to verify it

Exsiting e2e tests and additional unit tests

- Description for the changelog

Refactoring to support future alternatives to node reboots in the specific cases that we know it is safe to do so.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: beekhof
To complete the pull request process, please assign sinnykumari
You can assign the PR to them by writing /assign @sinnykumari in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

Drain and cordon is always performed
Currently the only exception is /etc/containers/registry.conf
Treat skipped systemd actions as errors and remove support for unused actions
Switch to more generic config arrangement
Fix formatting and remove dead code
Draining is unconditional, no need to do it twice
Add basic unit tests
Improved unit tests
Convert actionResult to a public interface so that we can compare them
Add additional unit tests for applying file based changes
Removed unneeded test inputs
Reduce the cyclomatic complexity of update() in pkg/daemon
Remove unnecessary use of Daemon from ActionResult
Log each action description in ActionResult tests
Prevent use-of-nil when finalizing MachineConfig updates
Add an e2e test for registry changes
Rename the interface for config update actions
Refactor MCD bootstrap finalize so that it can be reused for the non-reboots flow
Remove all strategies that do not invole reboots

Signed-off-by: Andrew Beekhof <andrew@beekhof.net>
@beekhof beekhof force-pushed the optional-reboot-prep branch from c82e8c9 to dd77098 Compare July 28, 2020 05:02
@beekhof
Copy link
Author

beekhof commented Jul 28, 2020

I can also go one further and strip out the inert systemd struct and code

@beekhof
Copy link
Author

beekhof commented Jul 28, 2020

/cc @runcom @ashcrow @cgwalters @crawford

@beekhof
Copy link
Author

beekhof commented Jul 29, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2020
@openshift-ci-robot
Copy link
Contributor

@beekhof: PR needs rebase.

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.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Looks like this is on the right track at a high level.

Needs a rebase 🏄


func calculateActions(stripPrefix string, oldConfig, newConfig *mcfgv1.MachineConfig, diff *machineConfigDiff) []ConfigUpdateAction {

if diff.osUpdate || diff.kargs || diff.fips || diff.kernelType {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps diff.isFilesystemOnly()?


// Check for any changes not already excluded by Reconcilable()
// Alternatively, fold this code into that function
if !reflect.DeepEqual(oldIgnConfig.Ignition, newIgnConfig.Ignition) {
Copy link
Member

Choose a reason for hiding this comment

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

We could also compute these in machineConfigDiff.

@openshift-ci-robot
Copy link
Contributor

@beekhof: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/unit dd77098 link /test unit
ci/prow/e2e-gcp-upgrade dd77098 link /test e2e-gcp-upgrade
ci/prow/e2e-aws-scaleup-rhel7 dd77098 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-aws dd77098 link /test e2e-aws
ci/prow/e2e-metal-ipi dd77098 link /test e2e-metal-ipi
ci/prow/e2e-upgrade dd77098 link /test e2e-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-merge-robot
Copy link
Contributor

@beekhof: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-agnostic-upgrade dd77098 link /test e2e-agnostic-upgrade
ci/prow/e2e-aws-serial dd77098 link /test e2e-aws-serial

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
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 4, 2021
@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 6, 2021
@openshift-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci-robot
Copy link
Contributor

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants