Disable locksmithd on CoreOS if UpdatePolicy set #4909

KashifSaadat · 2018-04-04T16:31:49Z

Disable locksmithd.service if running CoreOS, and the following is set within your kops ClusterSpec:

spec:
  updatePolicy: external

KashifSaadat · 2018-04-04T16:37:32Z

/assign @chrislovecnm @justinsb

Wasn't sure whether you'd prefer for this to be wrapped in an API flag? I noticed the flag UpdatePolicy, however the default value for that flag (missing) states applying upgrades automatically, so users would need to explicitly set the value to external if using CoreOS.

chrislovecnm · 2018-04-04T17:11:26Z

pkg/apis/kops/v1alpha1/zz_generated.conversion.go

@@ -2125,6 +2125,7 @@ func autoConvert_v1alpha1_KubeProxyConfig_To_kops_KubeProxyConfig(in *KubeProxyC
 	out.HostnameOverride = in.HostnameOverride
 	out.Master = in.Master
 	out.Enabled = in.Enabled
+	out.ProxyMode = in.ProxyMode


For some reason we are missing this ... we will need to get this into the 1.9 release as well. I am pushing a pr.

Ok great, cheers 👍 It somehow got past the tests and merged into master, but the test run in this PR failed.

euank

Heya, I work on CoreOS Container Linux and wanted to mention some other options worth considering.

First of all, we're sorry stable broke users here. We do our best to catch issues before they hit users, but the /dev/ path thing wasn't hit in our tests nor reported by users and so slipped through.

We obviously prefer if our users do remain updated and secure, so I think it would be preferable to do something a little less aggressive than masking both update-engine and locksmthid.

The options are basically the following (as discussed in the docs):

Disable update-engine + locksmithd — Updates are not downloaded or applied at any time
Disable locksmithd — Updates are downloaded and prepared, but wait for an alternate update-coordinator (such as CLUO) or other reboot (e.g. operator reboot or crash) to apply.
Configure locksmithd to use the etcd-lock (or off) strategy — Updates will only be applied if the operator further configures locksmithd or the machine is rebooted/crashes.

I think that option 2 may be preferable to option 1 since it makes it easier for a cluster operator to later migrate to using something like CLUO to coordinate updates in a Kubernetes-aware way.
The fact that new machines in the ASG will launch with the old ami will also provide a semi-natural rollback if things do go wrong and the user shuts down misbehaving machines, so breakages can be remediated fairly easily still.

Even though the ASG will still launch older machines, I do think providing an easier migration path to updating them forwards via leaving update-engine there is worthwhile.

As a final note, I think one additional thing that could help make sure updates are less likely to cause issues would be making it easier to run clusters on the beta channel (where users could run a staging / pre-prod cluster and catch issues earlier), or possibly make it possible to mix in some instances running on the beta channel.
I think if that option existed, stable updates would be less likely to cause problems for kops without advance warning since almost all stable changes are promoted through the beta channel.

Let me know if you have questions about the above.
Best,
Euan

euank · 2018-04-04T19:22:33Z

nodeup/pkg/model/update_service.go

+
+	manifest.Set("Unit", "Before", "update-engine.service locksmithd.service")
+	manifest.Set("Service", "Type", "oneshot")
+	manifest.Set("Service", "ExecStartPre", "/bin/sh -c 'systemctl mask --now update-engine.service && systemctl mask --now locksmithd.service'")


I notice this is being run for Google's Container-optimized OS as well. Does it have locksmithd.service? If it doesn't, this would fail there, and I'm not sure it does.

There may need to be two separate services

Ah yes good spot, I'll update and test

KashifSaadat · 2018-04-05T02:08:37Z

Hey @euank, many thanks for the detailed feedback! :)

Option 2 does sound like a more sensible approach in this scenario, and we could also add the ability for users to enable the CLUO deployment for their Cluster if they want it (although there's still the case that the ASG would launch an outdated AMI).

I'll make the suggested update to this PR, would also be good to hear others thoughts on CLUO and whether we should add that capability to deploy it for the user within Kops here.

Thanks!
Kash

justinsb · 2018-04-09T19:59:41Z

We could probably do an e2e test with CoreOS running on the beta channel - I don't think anything is stopping anyone from specifying a beta or alpha AMI, but we didn't learn of this until it was too late.

I guess this is correct; I think we probably should tie it into the existing UpdatePolicy field on the cluster though - by default we should probably continue to update as best we can, at least until we have a more managed update process. I'm not sure that in-place updates make sense on clouds / virtual infrastructure, but we don't currently have an operator that reflects that, and are still planning bare metal support.

@KashifSaadat would you be OK with tweaking this to key off UpdatePolicy? I don't want to surprise existing users that suddenly find their clusters are not getting updates, even if the current update strategy has caused problems..

chrislovecnm · 2018-04-09T22:12:17Z

@euank thanks for the review btw!

KashifSaadat · 2018-04-10T12:08:24Z

@justinsb sure, I've just updated the PR. Users will need to specify the following within their ClusterSpec for this change to take effect (only disabling locksmithd):

spec:
  updatePolicy: external

gambol99 · 2018-04-10T12:55:09Z

LGTM

gambol99 · 2018-04-10T12:57:42Z

/lgtm

k8s-ci-robot · 2018-04-10T12:57:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, KashifSaadat

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:

~~OWNERS~~ [KashifSaadat,gambol99]

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

jpds · 2018-05-09T11:30:49Z

This option doesn't appear to have been documented?

davidarcher · 2018-06-04T10:40:54Z

@jpds it's documented here although it still says that unset means no reboots:

https://github.com/kubernetes/kops/blob/master/docs/arguments.md#updatepolicy

seh · 2021-03-31T00:50:29Z

This patch came up in discussion in the "kops-users" Slack channel.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 4, 2018

KashifSaadat force-pushed the coreos-disable-updates branch from 9f4a42b to 843b82b Compare April 4, 2018 16:37

k8s-ci-robot assigned chrislovecnm and justinsb Apr 4, 2018

KashifSaadat force-pushed the coreos-disable-updates branch from 843b82b to 28c20a8 Compare April 4, 2018 16:38

chrislovecnm reviewed Apr 4, 2018

View reviewed changes

KashifSaadat mentioned this pull request Apr 4, 2018

etcd is not started on CoreOS-based masters since 1688.4.0 #4837

Closed

euank reviewed Apr 4, 2018

View reviewed changes

KashifSaadat force-pushed the coreos-disable-updates branch from 28c20a8 to fb3702b Compare April 9, 2018 08:25

Disable locksmithd on CoreOS if UpdatePolicy set

3d1203f

KashifSaadat force-pushed the coreos-disable-updates branch from fb3702b to 3d1203f Compare April 10, 2018 12:05

KashifSaadat changed the title ~~Disable update-engine by default on CoreOS~~ Disable locksmithd on CoreOS if UpdatePolicy set Apr 10, 2018

k8s-ci-robot assigned gambol99 Apr 10, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2018

k8s-ci-robot merged commit 7b601e6 into kubernetes:master Apr 10, 2018

KashifSaadat deleted the coreos-disable-updates branch April 10, 2018 13:35

KashifSaadat mentioned this pull request Jul 5, 2018

CoreOS - locksmithd semaphore on updates on local(non clustered) etcd, rebooting all nodes at once. #5407

Closed

johnzheng1975 mentioned this pull request Aug 10, 2018

Disable locksmithd on CoreOS if coreos_auto_upgrade set to false kubernetes-sigs/kubespray#3088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable locksmithd on CoreOS if UpdatePolicy set #4909

Disable locksmithd on CoreOS if UpdatePolicy set #4909

KashifSaadat commented Apr 4, 2018 •

edited

Loading

KashifSaadat commented Apr 4, 2018

chrislovecnm Apr 4, 2018

KashifSaadat Apr 4, 2018 •

edited

Loading

euank left a comment

euank Apr 4, 2018

KashifSaadat Apr 5, 2018

KashifSaadat commented Apr 5, 2018

justinsb commented Apr 9, 2018

chrislovecnm commented Apr 9, 2018

KashifSaadat commented Apr 10, 2018

gambol99 commented Apr 10, 2018 •

edited

Loading

gambol99 commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

jpds commented May 9, 2018

davidarcher commented Jun 4, 2018

seh commented Mar 31, 2021

Disable locksmithd on CoreOS if UpdatePolicy set #4909

Disable locksmithd on CoreOS if UpdatePolicy set #4909

Conversation

KashifSaadat commented Apr 4, 2018 • edited Loading

KashifSaadat commented Apr 4, 2018

chrislovecnm Apr 4, 2018

Choose a reason for hiding this comment

KashifSaadat Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

euank left a comment

Choose a reason for hiding this comment

euank Apr 4, 2018

Choose a reason for hiding this comment

KashifSaadat Apr 5, 2018

Choose a reason for hiding this comment

KashifSaadat commented Apr 5, 2018

justinsb commented Apr 9, 2018

chrislovecnm commented Apr 9, 2018

KashifSaadat commented Apr 10, 2018

gambol99 commented Apr 10, 2018 • edited Loading

gambol99 commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

jpds commented May 9, 2018

davidarcher commented Jun 4, 2018

seh commented Mar 31, 2021

KashifSaadat commented Apr 4, 2018 •

edited

Loading

KashifSaadat Apr 4, 2018 •

edited

Loading

gambol99 commented Apr 10, 2018 •

edited

Loading