-
Notifications
You must be signed in to change notification settings - Fork 39.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify the kubeadm upgrade DAG for the TLS Upgrade #62655
Modify the kubeadm upgrade DAG for the TLS Upgrade #62655
Conversation
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.
I'm going to stop reviews on this until we can chat.
However, I would like to trim out any refactorings that may be unnecessary in this PR.
@@ -23,7 +23,7 @@ import ( | |||
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" | |||
"k8s.io/kubernetes/cmd/kubeadm/app/features" | |||
"k8s.io/kubernetes/cmd/kubeadm/app/phases/addons/dns" | |||
"k8s.io/kubernetes/cmd/kubeadm/app/util" | |||
etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd" |
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.
I don't see what this change has todo with the this PR.
FWIW renaming and refactorings should not be in this PR.
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.
I neglected to put this in the commit, but it was necessary for me to restructure the module, because HasTLS()
imports ReadStaticPodFromDisk
from app/util/staticpod
app/util/staticpod
already imports from /app/util
and this created a dependency cycle.
// notice the removal of the Static Pod, leading to a false positive below where we check that the API endpoint is healthy | ||
// If we don't do this, there is a case where we remove the Static Pod manifest, kubelet is slow to react, kubeadm checks the | ||
// API endpoint below of the OLD Static Pod component and proceeds quickly enough, which might lead to unexpected results. | ||
if err := waiter.WaitForStaticPodHashChange(cfg.NodeName, component, beforePodHash); err != nil { |
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.
Doesn't this overlap with @detiber 's proposed kubelet change?
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.
It does, these are all proposed solutions that complement each other and overlap in different ways.
This PR skips the dependency on the APIServer for the protocol switch.
The kubelet API PR completely gets rid of the hard dependency on the APIServer.
@stealthybox @detiber - we need to get this done ASAP for 1.10.2 release! |
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process @detiber @stealthybox @timothysc Pull Request Labels
|
- Update kubeadm static pod upgrades to use the kubetypes.ConfigHashAnnotationKey annotation on the mirror pod rather than generating a hash from the full object info. Previously, a status update for the pod would allow the upgrade to proceed before the new static pod manifest was actually deployed. Signed-off-by: Jason DeTiberus <detiber@gmail.com>
14049c0
to
022cfd9
Compare
6e17c22
to
27514f4
Compare
@timothysc We should validate that it also allows for External Etcd during the upgrade such as in #62141 A final nice to have would be to add more test cases and reduce the differences between test behavior and actual behavior. |
@stealthybox There are still some additional changes needed to unblock external etcd, but I think I'd like to get that sorted out as a followup. |
/lgtm |
/cc @MaciekPytel - for milestone cherry-pick approval. |
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.
Please address comments.
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
for more eyeballs, b/c this is a large back-patch and we want to be certain here.
@@ -281,7 +282,9 @@ func PerformStaticPodUpgrade(client clientset.Interface, waiter apiclient.Waiter | |||
return err | |||
} | |||
|
|||
return upgrade.StaticPodControlPlane(waiter, pathManager, internalcfg, etcdUpgrade) | |||
// These are uninitialized because passing in the clients allow for mocking the client during testing | |||
var oldEtcdClient, newEtdClient etcdutil.Client |
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.
This is weird and would cause static analysis issues.
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.
I agree that this is weird.
The trouble we ran into is that we cannot initialize the etcd client before the TLS certs are created.
We could lazy initialize the client, but that sounds buggy as well.
We could also store test interfaces in a global var and conditionally initialize them instead of passing them in as params.
I'm not sure what the best way to address this is.
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.
We could pass the client creation code as functions?
WDYT?
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.
At this point let's just open an issue to follow up on some of these items.
@@ -61,14 +62,20 @@ func (f *fakeVersionGetter) KubeletVersions() (map[string]uint16, error) { | |||
}, nil | |||
} | |||
|
|||
type fakeEtcdCluster struct{} | |||
type fakeEtcdCluster struct{ TLS bool } |
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.
Could you please open an issue about instead of creating a stub to use the test jiggery that exists to auto-standup and cleanup etcd.
return c.TLS | ||
} | ||
|
||
func (c fakeTLSEtcdClient) GetStatus() (*clientv3.StatusResponse, error) { |
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.
same as other comment, I'm an anti-stub person and we have the jiggery to dynamically spin clusters in UTs.
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.
@timothysc this sounds great for testing GenericClient.GetStatus()
in util/etcd/etcd_test.go
Do we have an existing unit-test harness for running etcd as a static pod using a kubelet?
That would be what is required for this particular test.
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.
We have utilities to basically spin up an actual etcd server + client pair, but at this point I'm ok with merging what is here and we can create a new issue and cross-link for future work.
@@ -136,29 +141,36 @@ type fakeStaticPodPathManager struct { | |||
} | |||
|
|||
func NewFakeStaticPodPathManager(moveFileFunc func(string, string) error) (StaticPodPathManager, error) { | |||
realManifestsDir, err := ioutil.TempDir("", "kubeadm-upgraded-manifests") | |||
kubernetesDir, err := ioutil.TempDir("", "kubeadm-pathmanager-") |
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.
Who cleans up these directories?
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.
defer os.RemoveAll(pathMgr.(*fakeStaticPodPathManager).KubernetesDir())
https://github.com/kubernetes/kubernetes/pull/62655/files#diff-012a4c853bcc083f2ac6c61d69e7292eR375
} | ||
|
||
// GenericClient is a common etcd client for supported etcd servers | ||
type GenericClient struct { |
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.
Did you test the upgrade scenario of external etcd that @craigtracey was validating?
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.
We don't have a test for this, but I assume @detiber would include it in the follow up PR?
cmd/kubeadm/app/util/etcd/etcd.go
Outdated
func (c GenericClient) GetStatus() (*clientv3.StatusResponse, error) { | ||
cli, err := clientv3.New(clientv3.Config{ | ||
Endpoints: c.Endpoints, | ||
DialTimeout: 5 * time.Second, |
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.
const above.
// WaitForStatus returns a StatusResponse after an initial delay and retry attempts | ||
func (c GenericClient) WaitForStatus(delay time.Duration, retries int, retryInterval time.Duration) (*clientv3.StatusResponse, error) { | ||
fmt.Printf("[util/etcd] Waiting %v for initial delay\n", delay) | ||
time.Sleep(delay) |
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.
this is basically a wait.Until, please use that f(n) or comment to reduce this later.
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.
@timothysc, are you referring to this?
https://godoc.org/k8s.io/apimachinery/pkg/util/wait#Until
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.
stealthybox#2 attempts to modify WaitForStatus to use wait.Until from apimachinery.
@@ -0,0 +1,197 @@ | |||
/* | |||
Copyright 2017 The Kubernetes Authors. |
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.
2018
- Test HasTLS() - Instrument throughout upgrade plan and apply - Update plan_test and apply_test to use new fake Cluster interfaces - Add descriptions to upgrade range test - Support KubernetesDir and EtcdDataDir in upgrade tests - Cover etcdUpgrade in upgrade tests - Cover upcoming TLSUpgrade in upgrade tests
- Calculate `beforePodHashMap` before the etcd upgrade in anticipation of KubeAPIServer downtime - Detect if pre-upgrade etcd static pod cluster `HasTLS()==false` to switch on the Etcd TLS Upgrade if TLS Upgrade: - Skip L7 Etcd check (could implement a waiter for this) - Skip data rollback on etcd upgrade failure due to lack of L7 check (APIServer is already down unable to serve new requests) - On APIServer upgrade failure, also rollback the etcd manifest to maintain protocol compatibility - Add logging
- Adds L7 check for kubeadm etcd static pod upgrade
Fix `rollbackEtcdData()` to return error=nil on success `rollbackEtcdData()` used to always return an error making the rest of the upgrade code completely unreachable. Ignore errors from `rollbackOldManifests()` during the rollback since it always returns an error. Success of the rollback is gated with etcd L7 healthchecks. Remove logic implying the etcd manifest should be rolled back when `upgradeComponent()` fails
27514f4
to
dac4fe8
Compare
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.
/lgtm
/approve
@@ -281,7 +282,9 @@ func PerformStaticPodUpgrade(client clientset.Interface, waiter apiclient.Waiter | |||
return err | |||
} | |||
|
|||
return upgrade.StaticPodControlPlane(waiter, pathManager, internalcfg, etcdUpgrade) | |||
// These are uninitialized because passing in the clients allow for mocking the client during testing | |||
var oldEtcdClient, newEtdClient etcdutil.Client |
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.
At this point let's just open an issue to follow up on some of these items.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber, stealthybox, timothysc 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 62655, 61711, 59122, 62853, 62390). If you want to cherry-pick this change to another branch, please follow the instructions here. |
@stealthybox: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
…62655-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #62655: Modify the kubeadm upgrade DAG for the TLS Upgrade Cherry pick of #62655 on release-1.10. #62655: Modify the kubeadm upgrade DAG for the TLS Upgrade **Release note**: ```release-note Action Required: kubeadm upgrade no longer supports downgrading 1.10 clusters to 1.9 clusters due to an incompatibility between the kubernetes 1.9 and 1.10 featureGates struct. Please backup /etc/kubernetes/manifests, the etcd database, and the kubeadm-config configmap if you anticipate a need to rollback. ```
@stealthybox Shouldn't this now also work for external etcd clusters? Because i just updated kubeadm to 1.10.2 but i still get:
|
@discostur, I believe @detiber is still putting together support for external etcd. |
I have upgraded my kubeadm and kubectl version to v1.10.2 I am still not able to upgrade successfully. I get the below error: root # kubeadm upgrade apply v1.10.1 Can someone help me with this upgrade? any worarounds? |
@ypsingh27 it looks like your upgrade failed on the apiserver manifest. The first thing I think of is that you may not have been able to pull the image within the timeout. Somebody in the #kubeadm slack channel may be able to help you. |
@detiber are there any news on the failed upgrade with external etcd support? Because in the 1.10.2 release notes they write:
but it still fails :/ |
@stealthybox Please let me know, what more details are needed. Also how can I pe-download the manifest? Can I use the manifest of 1.10.1 from other system and restart kubelet? |
Just wanted to second the same issues @discostur is seeing. Any updates? |
What this PR does / why we need it:
This adds the necessary utilities to detect Etcd TLS on static pods from the file system and query Etcd.
It modifies the upgrade logic to make it support the APIServer downtime.
Tests are included and should be passing.
These cases are working consistently for me
This branch is based on top of #61942, as resolving the hash race condition is necessary for consistent behavior.
It looks to fit in pretty well with @craigtracey's PR: #62141
The interfaces are pretty similar
/assign @detiber @timothysc
Which issue(s) this PR fixes
Helps with kubernetes/kubeadm#740
Special notes for your reviewer:
278b322
[kubeadm] Implement ReadStaticPodFromDisk
c74b563
Implement etcdutils with Cluster.HasTLS()
8d8e5fe
Update test-case, fix nil-pointer bug, and improve error message
97117fa
Modify the kubeadm upgrade DAG for the TLS Upgrade
Calculate
beforePodHashMap
before the etcd upgrade in anticipation ofKubeAPIServer downtime
Detect if pre-upgrade etcd static pod cluster
HasTLS()==false
to switchon the Etcd TLS Upgrade if TLS Upgrade:
(APIServer is already down unable to serve new requests)
maintain protocol compatibility
Add logging
Release note: