Skip to content

enhancements/update/automatic-updates: Propose a new enhancement #124

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.

Sign up for GitHub

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

Closed
wking wants to merge 1 commit into from
Closed

enhancements/update/automatic-updates: Propose a new enhancement #124

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 176 additions & 0 deletions enhancements/update/automatic-updates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
---
title: neat-enhancement-idea
authors:
- "@wking"
reviewers:
- "@abhinavdahiya"
- "@crawford"
- "@smarterclayton"
approvers:
- TBD

@derekwaynecarr derekwaynecarr Jan 29, 2021

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.

suggest @crawford ?

creation-date: 2019-11-19
last-updated: 2019-11-21
status: implementable

@derekwaynecarr derekwaynecarr Jan 29, 2021

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.

possibly update this status to deferred given the frozen state if we want to merge for record keeping?

---

# Automatic Updates

## Release Signoff Checklist

- [x] Enhancement is `implementable`
- [x] Design details are appropriately documented from clear requirements
- [x] Test plan is defined
- [x] Graduation criteria for dev preview, tech preview, GA
- [ ] User-facing documentation is created in [openshift-docs](https://github.com/openshift/openshift-docs/)

## Summary

Operator-managed updates are [one of the key benefits of OpenShift 4][architecture].
Cluster update suggestions are distributed via [Cincinnati][cincinnati-openshift], so clusters only receive update recommendations that are appropriate for their cluster.
While manually approving updates at a per-cluster level is possible, it should not be required.
This enhancement adds a new `automaticUpdates` property to [the ClusterVersion `spec`][api-spec] so cluster administrators can opt in to or out of automatic updates.

## Motivation

Manually approving updates is tedious, doesn't scale well for administrators managing many clusters, and delays the application of potential security fixes.
Administrators may wish to enable automatic updates for any of those or other reasons.
Currently, administrators must implement custom polling logic to check for and apply any available updates.
This enhancement would provide those administrators with a convenient property instead.

### Goals

* Make it easy to opt in to and out of automatic updates.

### Non-Goals

@derekwaynecarr derekwaynecarr Jan 29, 2021

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.

for reference, openshift dedicated has layered logic over the cluster to handle calendar gating logic.

the update settings presented for dedicated are the following:

automatic with a user supplied preferred day and start time.
manual with a warning that critical and high cve vulnerabilities will be patched independent of this setting.

node draining budget for maximum grace time is also configurable (default 1 hr)


* Support calendar gating or other logic about when updates may be attempted.
* Choose whether the installer defaults to enabling automatic updates or not for new clusters.

## Proposal

As proposed in [this API pull request][api-pull-request], to add a new property:

```go
// automaticUpdates enables automatic updates.
// +optional
AutomaticUpdates bool `json:"automaticUpdates,omitempty"`
Comment on lines +54 to +56

@abhinavdahiya abhinavdahiya Nov 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think booleans are great API as they are not extensible..

having a boolean makes it so that we are saying that cvo only knows update always or just don't and it is never going to do anything else, any condition if required will be done somewhere else..

personally i think something like union discriminator kubernetes/enhancements#926 is much better suited to express the intent no-updates, auto-updates-always, auto-updates-schedule etc..

https://github.com/openshift/enhancements/pull/124/files#diff-8354eb4f233832a1e5afa3ffc9bea8e8R154-R160

There is an overhead trade-off that needs to taken into consideration before saying CVO doesn't do local stuff...
will a user with single or handful clusters want to run local policy-agent? or is it sane for them to do it?
will the users that want different policy for their clusters like testing/staging/prod like to run 3 different policy agents?

Does the fact that we have no policy agent at this point, and not sure how far that is or even the concept of supporting upstream that is self-signed... deter anybody from even using this... hence diminishing the value prop of the boolean.

Personally i would like clusters to support at some one usecase of automatic updates based on condition..

@wking wking Nov 26, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think booleans are great API as they are not extensible..

I'm still not clear on what sort of extensibility you'd want. I think there's a set of policy engines which can be distributed in external chains or run inside the CVO itself, that eventually spit out a set of recommended update targets. Then you need a boolean knob to decide if the CVO automatically initiates an update when a target becomes available. For example:

  1. Upstream Cincinnati spits out graph A.
  2. On-site policy engine ingests graph A and applies local stability filtering, removing Red-Hat-recommended update edges until they have also passed local update testing. This engine spits out graph B.
  3. CVO ingests graph B via the upstream URI, and applies schedule filtering, removing all update edges if and only if the current time lies outside the configured update windows. This engine spits out graph C.
  4. CVO ingests its own graph C and looks for available updates from the current version, storing those in AvailableUpdates.
  5. CVO looks at AvailableUpdates and the AutomaticUpdates boolean, and decides whether to automatically set DesiredUpdate.

So there's still lots of room for extension. The boolean option is just an in-cluster knob for the existing CLI flag.

...any condition if required will be done somewhere else...

No, you could still apply policy filtering on the CVO side, independent of whether the CVO automatically updates based on the results of a local policy engine. This is what I was trying to convey here.

```

to [`ClusterVersionSpec`][api-spec] to record the administrator's preference.

### User Stories

#### Leading Clusters

Alice has clusters she wants to update as soon as an updated release is available in her configured channel.
Before this enhancement she would need to construct a poller to run `oc adm upgrade --to-latest` or similar.
With this enhancement, she can set `automaticUpdates` and does not need to write an external poller.

### Risks and Mitigations

Cluster-managed updates are a relatively new thing, and while we have dozens of OpenShift 4 releases out so far, we have had a few updates result in stuck clusters (like [this][machine-config-operator-drain-wedge].
Allowing automatic updates would make it more likely that update failures happen when there is no administrator actively watching to notice and recover from the failure.
This is mitigated by:

* Alerting.

@brancz brancz Dec 12, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We actually know that certain (non critical) alerts will fire during upgrades. The observability group (of which the in-cluster monitoring team is a part of) has been trialing silencing everything but critical alerts during upgrades. While this is not perfect yet, I think this is what we ultimately want to automate for automatic upgrades.

That said any severity: critical alerts firing should prevent an automatic upgrade from happening. This level is used to page people, so in a production cluster this should already mean that someone has been notified. Our goal must be that no critical alerts are firing during upgrades. Essentially this means we need to set certain objectives per component and if they are violated we cannot proceed with an automatic upgrade and/or people are notified. I know the in-cluster team is planning an initiative to introduce these objectives for components (at this point earliest in the 4.5 time frame I would say).

@wking wking Feb 7, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We actually know that certain (non critical) alerts will fire during upgrades.

Do we consider these overly sensitive alerts? Update bugs? I'd expect that our goal is to have updates be smooth enough that we can apply them without additional alerts firing. Silencing non-critical alerts sounds like a reasonable stopgap to avoid alert fatigue, but I'm less comfortable with it as a long-term plan.

That said any severity: critical alerts firing should prevent an automatic upgrade from happening.

Hmm. We might be able to swing this with the monitoring operator setting Upgradeable=False when it sees a critical alert.

@brancz brancz Feb 18, 2020 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Silencing is made exactly for the case of "we know a maintenance window is coming, so don't bother us with non critical things", so I think it's exactly the right thing. Warning alerts are more the type of "something might be up, but it's not urgent". They should typically just open a ticket for someone to eventually take a look at. Critical alerts are the type of alerts that tell someone that there is something that needs attention urgently and that SLOs are likely to be violated. The later should never fire during an upgrade, I agree.

We might be able to swing this with the monitoring operator setting Upgradeable=False when it sees a critical alert.

I think that's a good idea.

The cluster should fire alerts (FIXME: which ones?) when an update gets into trouble.
Administrators can configure the cluster to push those alerts out to the on-call administrator to recover the cluster.
* Stability testing.
We are continually refining our CI suite and processing Telemetry from live clusters in order to assess the stability of each update.
We will not place updates in production channels unless they have proven themselves stable in earlier testing, and we will remove update recommendations from production channels if a live cluster trips over a corner case that we do not yet cover in pretesting.

@smarterclayton smarterclayton Nov 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Without the ability to control rollout, this seems like a risky feature. What is the minimal phased rollout feature that mitigates risk?

@wking wking Nov 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What is the minimal phased rollout feature that mitigates risk?

The ability to spread the update out across a release-admin-specified time window, with clusters in the given channel randomly spread across that window. Things like automatically pulling edges on failure would be nice, but can be mitigated with manual monitoring and large-duration windows (i.e. slow rollouts). Things like intelligently sorting sensitive clusters toward the back of the queue would also be nice, but can be mitigated by ensuring sufficient populations in less-stable channels.


There are also potential future mitigations:

* Phased rollouts, where Cincinnati spreads an update suggestion out over a configurable time window.
With hard cutovers and automatic updates, many clusters in a given channel could attempt a new update simultaneously.
If that update proves unstable, many of those updates would already be in progress by the time the first Telemetry comes back with failure messages.
A phased rollout would limit the number of simultaneously updating clusters to give Telemetry time to come back so we could stop recommending updates that proved unstable on live clusters not yet covered in pretesting.

There is also a security risk where a compromised upstream Cincinnati could recommend cluster updates that were not in the cluster's best interest (e.g. 4.2.4 -> 4.1.0).

@lucab lucab Nov 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure if relevant for OpenShift mitigations, but Zincati has client-side checks and knobs to prevent auto-downgrades: https://github.com/coreos/zincati/blob/dbb0b0a8884435f2b1186b2228199cd4adb6f705/docs/usage/auto-updates.md#updates-ordering-and-downgrades

@wking wking Nov 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure if relevant for OpenShift mitigations...

We could grow this. But sometimes you might want to recommend a downgrade. E.g, 4.y.7 turns out to explode after 24h because of broken cert rotation, and a fixed 4.y.8 is 48h out, so you recommend 4.y.7->4.y.6 until then to get folks back to a safe place.

@ericavonb ericavonb Dec 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to version updates separately from the cluster versions, e.g.

update-1.0 => (ocp-4.y.6 -> ocp-4.y.7)
update-1.1 => (ocp-4.y.7 -> ocp-4.y.6)
update-1.2 => (ocp-4.y.6 -> ocp-4.y.8, ocp-4.y.7 -> ocp-4.y.8)

You always want the latest update version, which may or may not upgrade you to the highest cluster version.

Another way to address rollback attacks, as well as freeze attacks: TUF uses timestamps to certify updates for short durations, requiring constant refreshing to ensure the latest update metadata. Timestamp ordering is simple enough but does require secure and accurate time sources (for TUF, see the timestamp.json section: tuf-spec.md#4-document-formats

@cgwalters cgwalters Feb 7, 2020

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.

For libostree the timestamp in the commit is covered by the GPG signature; we don't do anything about comparing with the system wall clock, just require that the timestamp in the new commit increases.

The TUF threat model helps in a DoS attack where a MITM attacker just tells you there are no more updates. This is useful, but comes with a lot of overhead.

@cgwalters cgwalters Feb 7, 2020

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.

For OpenShift though the MCO ignores that bit, and obviously the libostree part isn't use for the rest of the container images anyways.

So I guess I'm just arguing that "signed timestamps" work pretty well and would likely be easy to add to the CVO if it doesn't do it today.

@wking wking Feb 7, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You always want the latest update version, which may or may not upgrade you to the highest cluster version.

We've talked about signing update recommendations, but you'd probably need to sign each of them separately. E.g.

from: 4.3.0
to: 4.3.1
expires: 2020-02-08T00:00Z

That's possible, but would be a fair bit of work to put in. Protections like "require admin overrides before applying downgrades" are coarser (and as above, sometimes you want downgrades), but would be easier to implement in the short term.

So I guess I'm just arguing that "signed timestamps" work pretty well and would likely be easy to add to the CVO if it doesn't do it today.

For the releases themselves, we have a version number and creation timestamp, both of which are covered by the image signature. Sorting on version number seems more sane to me, because we're cutting 4.2.18 after 4.3.0, and 4.3.0 -> 4.2.18 is probably not what most clusters want ;).

@cgwalters cgwalters Feb 7, 2020

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.

Right...when you have multiple branches you do want something more sophisticated. We ended up implementing ref binding in libostree, which avoids parsing the version strings. But I guess for the fully general case of switching branches, you do need something that is parsing them.

This is mitigated by:

* [The configurable `upstream` property][api-upstream].
Administrators who do not trust the default `upstream` to be sufficiently reliable may point their clusters at an upstream that they control which they can secure to their satisfaction.
The drawback to this approach would be that now they need tooling to approve graph updates as Red Hat makes changes, which will introduce some delays to security-fix rollouts.
But that approval would scale per-custom-Cincinnati instead of scaling per-cluster.

There are also potential future mitigations:

* The cluster-version operator could be taught to limit automatic updates to those which appear in the [signed metadata][cluster-version-operator-release-metadata] of either the source or target release.
The drawback to this approach is that updates would be limited to those expected when the releases were created and signed.

## Design Details

### Test Plan

In addition to unit tests, this feature will be tested in a fleet of canary clusters which will run with automatic updates enabled.
Having the canaries successfully and automatically update in to and out of a candidate release will be part of release and update stabilization criteria.
Comment thread
wking marked this conversation as resolved.
The "out of" testing is important, because we cannot strand production clusters on dead-end releases.
When testing a new candidate release B, the full loop for a short-lived test could be:

1. Launch a cluster using a previously-accepted release A, with automatic updates enabled and an [`upstream`][api-upstream] pointing at a per-test service.
2. Add the candidate release B to the per-test upstream, with a recommended update edge from A to B.
3. Watch the cluster successfully update from A to B.
4. Replace the A to B update edge with a B to A update edge in the per-test upstream.
5. Watch the cluster successfully update from B to A.

We might have update edges that are not reversible, so we don't want that A->B->A test to be blocking, but when we decide to stabilize a release candidate where B->A is unstable, we'd want to perform additional verification to convince ourselves that we weren't creating a dead end.

### Graduation Criteria

The ClusterVersion object is already GA, so there would be nothing to graduate or space to cool a preview implementation.

### Upgrade / Downgrade Strategy

The YAML rendering of the old and updated type are compatible, with the only difference being downgrades to earlier releases (where the property was not available) will clear any stored value.
Upon updating back to a version where the property is available, administrators would need to manually set it to restore automatic update behavior.

### Version Skew Strategy

As a boolean property, this would be [`false` by default][go-zero-values] on updates to the updated ClusterVersion Custom Resource Definition.
This preserves semantics for existing clusters, where manual updates are the only option.

The installer or administrator could set `automaticUpdates` `true` at cluster-creation time if they wished to have it enabled out of the box on new clusters.

## Implementation History

* [API pull request][api-pull-request].

## Drawbacks

The drawbacks are increased exposure to [automatic-update risks](#risks-and-mitigations).

## Alternatives

A [schedule structure][schedule-proposal] like:

```yaml
automaticUpdates:
schedule:
- Mon..Fri *-2..10-* 10:00,14:00
- Tues..Thurs *-11..1-* 11:00
```

The schedule structure was designed to support maintenance windows, allowing for updates on certain days or during certain times.

@lucab lucab Nov 20, 2019 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The entries shown above are punctual datetimes, not maintenance windows.
That is, they specify when a maintenance event can start (edge-trigger) but not how long it can be active (level-trigger). I fear this is a relevant semantic detail if there is any polling logic involved.

@wking wking Nov 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is, they specify when a maintenance event can start (edge-trigger) but not how long it can be active (level-trigger).

There's no cap on upgrade duration. If you're concerned about it, you'd have small windows early in your day for initiating upgrades, to leave lots of time for monitoring/recovering before you went home. But this is an alternative proposal; I dunno how deeply we want to dig into it here.

@lucab lucab Nov 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh sorry, I realize my comment was not worded clearly enough. I'm not trying to cap upgrade duration.

I'm stating that the schedule format would need to define a timespan, even just for the starting event.
That is, "allow auto-update events between 02:00 and 03:00 on Sat" instead of "allow auto-update events exactly at 02:00:00.00 on Sat".

The current syntax is taken from systemd timestamps, which defines a single specific point in time instead.

But I guess all of these are implementations details, so feel free to defer this discussion.

But we could enforce maintenance windows on a cluster independently of configuring automatic updates.
For example, a `schedule` property directly on [the ClusterVersion `spec`][api-spec] would protect administrators from accidentally using the web console or `oc adm upgrade ...` to trigger an update outside of the configured window.
Instead, administrators would have to adjust the `schedule` configuration or set an override option to trigger out-of-window updates.
Customization like this can also be addressed by intermediate [policy-engine][cincinnati-policy-engine], without involving the ClusterVersion configuration at all.
Teaching the cluster-version operator about a local `schedule` filter is effectively like a local policy engine.

@lucab lucab Nov 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is a difference though:

  • if a schedule is enforced locally by CVO, the operator can grow logic for an escape hatch for manual forced-updates (or only apply to auto-updates at all)
  • if a schedule is enforced globally by policy-engine, it cannot be force-bypassed locally

@wking wking Nov 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if a schedule is enforced globally by policy-engine, it cannot be force-bypassed locally

Sure it can, you just set the update by pullspec and force the "don't worry if this is not in your graph" exception.

I'm not against that, but it seems orthogonal to an automatic update property.

## Infrastructure Needed

There are already canary clusters will polling automatic updates, so we can use those for testing and will not need to provision more long-running clusters to excercise this enhancement.
We can provision additional short-lived clusters in existing CI accounts if we want to provide additional end-to-end testing.

[api-pull-request]: https://github.com/openshift/api/pull/326

@LalatenduMohanty LalatenduMohanty Nov 25, 2019

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.

This pull request is now closed. So may be we can remove it from here. Not sure what is the right action here.

@wking wking Nov 25, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This pull request is now closed.

It was closed pending this enhancement discussion. If the enhancement lands, I'll re-open and reroll the PR.

[api-spec]: https://github.com/openshift/api/blob/082f8e2a947ea8b4ed15c9c0f7b190d1fd35e6bc/config/v1/types_cluster_version.go#L28-L73
[api-upstream]: https://github.com/openshift/api/blob/082f8e2a947ea8b4ed15c9c0f7b190d1fd35e6bc/config/v1/types_cluster_version.go#L56-L60
[architecture]: https://docs.openshift.com/container-platform/4.1/architecture/architecture.html#architecture-platform-management_architecture
[cincinnati-openshift]: https://github.com/openshift/cincinnati/blob/c59f45c7bc09740055c54a28f2b8cac250f8e356/docs/design/openshift.md
[cincinnati-policy-engine]: https://github.com/openshift/cincinnati/blob/c59f45c7bc09740055c54a28f2b8cac250f8e356/docs/design/cincinnati.md#policy-engine
[cluster-version-operator-release-metadata]: https://github.com/openshift/cluster-version-operator/blob/0386842157d4db5d27ab5935db3cb69c52687d9d/pkg/payload/payload.go#L86
[go-zero-values]: https://golang.org/ref/spec#The_zero_value
[machine-config-operator-drain-wedge]: https://bugzilla.redhat.com/show_bug.cgi?id=1761557
[schedule-proposal]: https://github.com/openshift/api/pull/326#issuecomment-500081307