-
Notifications
You must be signed in to change notification settings - Fork 519
fix: Re-ordering HNS policy removal due to 10c change in behavior and consolidating logic in Windows cleanup scripts #4002
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4002 +/- ##
=======================================
Coverage 73.69% 73.69%
=======================================
Files 147 147
Lines 23164 23164
=======================================
Hits 17070 17070
Misses 4979 4979
Partials 1115 1115 Continue to review full report at Codecov.
|
@@ -49,6 +49,17 @@ if ($global:EnableHostsConfigAgent) { | |||
# Perform cleanup | |||
# | |||
|
|||
Write-Log "Cleaning up persisted HNS policy lists" | |||
# Workaround for https://github.com/kubernetes/kubernetes/pull/68923 in < 1.14, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these two workaround comments or say that the October patch impact ALL versions to make sure this doesn't get accidently cleaned up in the future?
@jsturtevant should we figure out a way to have kubeletstart.ps1 call into a portion of the windowsnodereset script so the contents will not drift again? I'm thinking maybe add a -CleanupOnly flag to windowsnodereset.ps1 that just stops services and performs the cleanup. Either that or create a cleanup script that both of these existing scripts calls. |
Adding a |
757d491
to
ce8d812
Compare
added |
After testing, the approach of using
|
843c218
to
72cbca1
Compare
I've updated to use a common file that only cleans up the network and other files. It is up to the caller to turn off the services required. |
These changes passed a local test run on 1.19 on a 10c image
|
/lgtm |
72cbca1
to
25666ab
Compare
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsturtevant, marosset The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
… consolidating logic in Windows cleanup scripts (#4002) * remove policy list before network * using common network clean up logic * add timeout for e2e tests to run
Reason for Change:
First step in refactoring shared logic in startup and windows node reset. We learned in 10c update (#3956) adds a breaking change that requires
Get-HnsPolicyList | Remove-HnsPolicyList
to be removed before HNS-Network. We also need turn off kubeproxy while doing this.This also adds removed containerd images during the kubelet startup.
We should be able to have a follow up with a change that shares logic between startup and windowsnodereset. This needs a little more consideration so is not included here.
Issue Fixed:
Credit Where Due:
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
Requirements:
Notes:
Requires signed scripts before kicking off tests
/hold