Skip to content

Mcd encap prep cleanups 1#1004

Merged
openshift-merge-robot merged 4 commits intoopenshift:masterfrom
cgwalters:mcd-encap-prep-cleanups-1
Jul 24, 2019
Merged

Mcd encap prep cleanups 1#1004
openshift-merge-robot merged 4 commits intoopenshift:masterfrom
cgwalters:mcd-encap-prep-cleanups-1

Conversation

@cgwalters
Copy link
Copy Markdown
Member

Extracted from #935

With one other small commit on top.

Would like to land this so I'm less in rebase hell later...

In once-from right now we have no previous config; the logic
here is now cleaner as the higher level code passes `nil`, then
the update logic can more easily understand there is no previous
state rather than checking `dn.onceFrom`.
Rather than stashing it in the daemon state, pass it down one
level via API.  Change other logic to check for e.g. `kubeClient`
as a signal for being cluster driven.

Related to this, reorder the `reboot` function logic slightly so
that we return early if `skipReboot` is set, rather than logging
that we're rebooting and then not doing it.

Prep for work on initial kargs, also just general logic cleanup.
Both callers outside of the unit test were passing the same things
and also duplicating the rationale; lower that into the function.

Drop the unit test since it really isn't useful enough to motivate
passing extra parameters in the primary path.
We weren't necessarily "killed by kube", maybe we got OOM killed
or crashed, etc.  It's about being interrupted in the middle
of a drain, so let's clarify the log message.
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2019
Comment thread pkg/daemon/daemon.go
// take a stab at that and re-run the drain+reboot routine
if state.pendingConfig != nil && bootID == dn.bootID {
dn.logSystem("killed by kube, retrying...")
dn.logSystem("drain interrupted, retrying")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for this, I completely forgot to clean this up

@runcom
Copy link
Copy Markdown
Member

runcom commented Jul 24, 2019

/approve

I'm not sure we can land this even if it's just a cleanup and it's far better than before - I'll check that out and/or create a master-4.3 branch otherwise where we can land these kind of PRs from now on

@cgwalters
Copy link
Copy Markdown
Member Author

It's just feature freeze, not code freeze right?

@runcom
Copy link
Copy Markdown
Member

runcom commented Jul 24, 2019

It's just feature freeze, not code freeze right?

right 🤔 this isn't changing anything then or adding anything

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 1a5275b into openshift:master Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants