strategy: add new `marker_file` strategy #540

kelvinfan001 · 2021-04-28T21:13:01Z

Add a simple, flexible strategy that checks /var/lib/zincati for
a JSON file (optionally containing an expiry timestamp key) named
allowfinalize.json and allows update finalization if such file
exists and has the proper permissions.

See #245 (comment) for the rationale for this new strategy.

cgwalters

Seems sane to me!

src/main.rs

src/strategy/mod.rs

src/strategy/filesystem.rs

cgwalters · 2021-04-28T22:27:11Z

src/strategy/filesystem.rs

+    let mode = attr.permissions().mode();
+
+    // Octal 100644 represents regular file, rw-r--r--.
+    if mode != 0o100644 {


I may have missed it, why are we strictly requiring 0644?

I thought this would be to prevent non-root users from being able to modify it, since this directly affects reboots.

Edit: I found out that as long as we set the directory to be owned by Zincati and not writable by others that'll satisfy our requirements for not letting non-root users add and remove files. So we just need to require it to be a regular file and not writable by others.

src/strategy/filesystem.rs

Makefile

lucab · 2021-04-29T11:14:19Z

I'd like to flush out the current pile of changes into a release before getting into reviewing this.

Additionally, this should come with some docs for users.

kelvinfan001 · 2021-04-29T12:07:43Z

@lucab yep, still working on the docs and possibly some kola tests. (sorry forgot to apply on-hold label).

src/strategy/filesystem.rs

kelvinfan001 · 2021-04-30T20:58:42Z

Notes:
After this basic functionality is merged, we should consider adding an inotify watch on the allowfinalize.json file to tick now so reboots happen immediately if an update is staged.
Also consider extending the marker file with an additional persistAfterReboot key where Zincati will remove the file before rebooting if false.

kelvinfan001 · 2021-05-01T19:08:09Z

src/strategy/marker_file.rs

+        anyhow::bail!("file is not regular file");
+    }
+
+    ensure_owned_by_root(&attr)?;


Do we still need this check if /var/lib/zincati is owned by the zincati user already anyway?

No, we trust systemd-tmpfiles for that (also, it's probably fine if the hierarchy is owned by root either).

docs/usage/updates-strategy.md

dustymabe · 2021-05-19T13:25:14Z

docs/usage/updates-strategy.md

+    - named `allowfinalize.json`
+    - under `/var/lib/zincati/admin/strategy/marker_file`


- named `allowfinalize.json` - under `/var/lib/zincati/admin/strategy/marker_file`

I wonder how this will work if you have multiple containers that might need to control reboot. If they are all trying to edit a single file then it gets problematic. Could or should we support multiple files under here?

That's a legit point but I'd say it's outside of the scope of this strategy. Specifically, here we have a bias towards a single controller who can be audited/blamed for finalization decisions.

Multiple un-coordinated agents are a likely interesting case but it possibly needs either 1) higher-level coordination, or 2) a different approach (i.e. some kind of rule to determine who makes the final go/no-go call).

lucab · 2021-06-01T14:10:42Z

docs/usage/updates-strategy.md

+If any of the above is not satisfied in your marker file, Zincati will not allow reboots.
+
+`allowfinalize.json` can optionally contain an `allowUntil` key with a Unix timestamp integer as its value to indicate the expiry date and time of this marker file. If the current time timestamp is _greater than or equal to_ this timestamp, then reboots will not be allowed.
+Otherwise, if the `allowUntil` key is not present, reboots will be allowed for as long as `allowfinalize.json` exists (in the right location), and it must be removed to disallow reboots.


Thinking aloud: do we need this to be optional right now? I am not fully sold on this way to say "allow all finalizations forever". Should we maybe start with making this mandatory and relax this to an Option later on if there is a need to?

I was thinking that the ability to leave a (valid) file in the directory forever would allow for the use case of "allow reboots for the most part unless I need to disallow it for a while". So for example, if theoretically Dusty's containers had some central controller, that controller could easily make use of this new strategy.

lucab · 2021-06-01T14:12:05Z

docs/usage/updates-strategy.md

+| sudo tee /var/lib/zincati/admin/strategy/marker_file/allowfinalize.json
+```
+
+Warning: In `jq` versions `1.6` and lower, `jq` [may output incorrect Unix timestamps][jq_bug] for certain datetimes on machines with certain `localtime`s.


This sounds scary enough, do we want fromdateiso8601 maybe instead?

I think fromdateiso8601 is the same as fromdate right now, but right, I think fromdateiso8601 would be more explicit.

lucab · 2021-06-01T14:36:20Z

src/strategy/marker_file.rs

+        anyhow::bail!("file is not regular file");
+    }
+
+    ensure_owned_by_root(&attr)?;


No, we trust systemd-tmpfiles for that (also, it's probably fine if the hierarchy is owned by root either).

lucab · 2021-06-01T15:11:29Z

Dusty was commenting out-of-band that the name here is still somehow suboptimal, possibly because still too generic.
I do share the same feeling, but I didn't come up with better ideas so far.

/cc @dustymabe

lucab · 2021-06-01T15:17:51Z

@kai-uwe-rommel we do envision this strategy (name still to be finalized) to provide the basic primitive for a central manager (like Ansible) pushing finalization markers to individual hosts.
I tried to put together a very simple playbook to see how the integration could look like, at https://github.com/lucab/ansible-zincati-marker-file (no fancy stuff, just basic file writing/deleting).
Can you give a look at all of this and see if it would generally fit into your expected flow?

dustymabe · 2021-06-01T15:27:02Z

Dusty was commenting out-of-band that the name here is still somehow suboptimal, possibly because still too generic.
I do share the same feeling, but I didn't come up with better ideas so far.

Yes. We were discussing this in the context of default allow versus default disallow (inhibit). This name is only sub-optimal if it doesn't cover both of those scenarios (which it currently doesn't) and we want to add another strategy later for the default to allow case in which case we might want to make it more obvious what the difference is between the two strategies.

Random ideas:

green_file (green light on a stop light for "go") - red_file in the future if we want to support disallowing update via file strategy
allow_file/disallow_file

I'm not really in love with any of them.

kelvinfan001 · 2021-06-01T17:29:48Z

We were discussing this in the context of default allow versus default disallow (inhibit). This name is only sub-optimal if it doesn't cover both of those scenarios (which it currently doesn't) and we want to add another strategy later for the default to allow case in which case we might want to make it more obvious what the difference is between the two strategies.

Sorry if this had already been discussed/clarified before elsewhere, but I still feel like default allow and default disallow is not too important here for this strategy. In theory, if there were a reliable single controller that is in charge of writing and removing a marker file, then the "defaults" for this strategy is totally up to the controller, right? I do recall vaguely it being mentioned that it is easier to touch a file than to remove a file, or vice versa, and so there might be some preference around whether we would like to default to Zincati being stuck in either constantly allowing reboots or constantly denying reboots. But I feel like this is a separate issue.

I think the key point for this strategy, in my opinion, is the emphasis that there should be a single controller that is the single decider on whether finalizations are allowed. If multiple processes/containers wanted to express their intent to allow/disallow reboots, that should be communicated to the central controller, rather than to Zincati's update strategy directly. Let me know if I've misunderstood anything :)

kai-uwe-rommel · 2021-06-05T17:24:50Z

@kai-uwe-rommel we do envision this strategy (name still to be finalized) to provide the basic primitive for a central manager (like Ansible) pushing finalization markers to individual hosts.
I tried to put together a very simple playbook to see how the integration could look like, at https://github.com/lucab/ansible-zincati-marker-file (no fancy stuff, just basic file writing/deleting).
Can you give a look at all of this and see if it would generally fit into your expected flow?

Thanks. Yes, that looks good, i.e. practical. It would allow us to explicitly upgrade a bunch of machines via a central action (Ansible) within a defined maintenance window.
In our case, these machines are load balancers and NFS servers for OCP/OKD clusters.
So we need to strictly adhere to defined maintenance windows.
Currently, we maintain the upgrades of the machines manually ...

kelvinfan001 · 2021-06-30T13:56:37Z

@lucab I think this should be ready for another look as well.

dustymabe · 2021-07-21T18:26:15Z

@lucab I think this should be ready for another look as well.

bump

Add a simple, flexible strategy that checks the directory `/var/lib/zincati/admin/marker_file_strategy` for a JSON file (optionally containing an expiry timestamp key) named `allowfinalize.json` and allows update finalization if such file exists and has the proper permissions.

This explains how the `marker_file` strategy works and how to configure it.

mysticaltech · 2022-01-26T19:57:38Z

Will be very useful! Keep up the good work folks.

dustymabe · 2023-03-20T14:22:44Z

I'd like to revive this at some point. The PR was closed but it doesn't say who or why it was closed.

jlebon · 2023-03-20T15:12:01Z

I'd like to revive this at some point. The PR was closed but it doesn't say who or why it was closed.

I think what happened there is that the fork was closed, so GitHub auto-closed all PRs from it.

kelvinfan001 added kind/new-feature area/updates labels Apr 28, 2021

cgwalters reviewed Apr 28, 2021

View reviewed changes

lucab added this to the vNext milestone Apr 29, 2021

lucab added the status/on-hold label Apr 29, 2021

lucab reviewed Apr 29, 2021

View reviewed changes

src/strategy/filesystem.rs Outdated Show resolved Hide resolved

src/strategy/filesystem.rs Outdated Show resolved Hide resolved

src/strategy/filesystem.rs Outdated Show resolved Hide resolved

lucab mentioned this pull request Apr 29, 2021

New strategy for background update but interactive reboots #539

Open

kelvinfan001 removed the status/on-hold label Apr 30, 2021

kelvinfan001 commented May 1, 2021

View reviewed changes

kelvinfan001 mentioned this pull request May 17, 2021

Slight UX issue with broadcast interactive terminal notification about upgrade #554

Closed

kelvinfan001 changed the title ~~strategy: add new filesystem strategy~~ strategy: add new marker_file strategy May 17, 2021

lucab modified the milestones: v0.0.21, vNext May 18, 2021

dustymabe reviewed May 19, 2021

View reviewed changes

docs/usage/updates-strategy.md Show resolved Hide resolved

dustymabe reviewed May 19, 2021

View reviewed changes

docs/usage/updates-strategy.md Outdated Show resolved Hide resolved

dustymabe reviewed May 19, 2021

View reviewed changes

kelvinfan001 linked an issue May 20, 2021 that may be closed by this pull request

updates: new strategy based on local filesystem #245

Open

lucab added the status/on-hold label May 25, 2021

lucab mentioned this pull request Jun 1, 2021

Unable to disable zincati.service using Ignition coreos/fedora-coreos-tracker#392

Closed

lucab reviewed Jun 1, 2021

View reviewed changes

kelvinfan001 removed the status/on-hold label Jun 29, 2021

lucab modified the milestones: v0.0.22, vNext Jul 7, 2021

lucab modified the milestones: v0.0.23, vNext Aug 5, 2021

kelvinfan001 added 3 commits August 20, 2021 17:14

docs/update-strategy: explain marker_file strategy

23d8e57

This explains how the `marker_file` strategy works and how to configure it.

kola/server: add e2e test for marker_file strategy

db90bcb

travier mentioned this pull request Sep 1, 2021

Feature Request: Make possible to pin target release coreos/fedora-coreos-tracker#947

Open

lucab modified the milestones: v0.0.24, vNext Nov 1, 2021

jlebon mentioned this pull request Nov 24, 2021

Check for Tang connectivity before proceeding with an update #687

Closed

lucab mentioned this pull request Jan 26, 2022

[design] Integration with Fedora CoreOS auto-updates (Zincati strategy) kubereboot/kured#216

Closed

jokajak mentioned this pull request Oct 9, 2023

strategy: add new marker_file strategy #1103

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

strategy: add new `marker_file` strategy #540

strategy: add new `marker_file` strategy #540

kelvinfan001 commented Apr 28, 2021 •

edited

Loading

cgwalters left a comment

cgwalters Apr 28, 2021

kelvinfan001 Apr 29, 2021 •

edited

Loading

lucab commented Apr 29, 2021

kelvinfan001 commented Apr 29, 2021

kelvinfan001 commented Apr 30, 2021

kelvinfan001 May 1, 2021

lucab Jun 1, 2021

dustymabe May 19, 2021

lucab Jun 1, 2021

lucab Jun 1, 2021

kelvinfan001 Jun 4, 2021

lucab Jun 1, 2021

kelvinfan001 Jun 4, 2021

lucab Jun 1, 2021

lucab commented Jun 1, 2021

lucab commented Jun 1, 2021

dustymabe commented Jun 1, 2021

kelvinfan001 commented Jun 1, 2021 •

edited

Loading

kai-uwe-rommel commented Jun 5, 2021

kelvinfan001 commented Jun 30, 2021

dustymabe commented Jul 21, 2021

mysticaltech commented Jan 26, 2022

dustymabe commented Mar 20, 2023

jlebon commented Mar 20, 2023

		- named `allowfinalize.json`
		- under `/var/lib/zincati/admin/strategy/marker_file`

strategy: add new marker_file strategy #540

strategy: add new marker_file strategy #540

Conversation

kelvinfan001 commented Apr 28, 2021 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvinfan001 Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

lucab commented Apr 29, 2021

kelvinfan001 commented Apr 29, 2021

kelvinfan001 commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jun 1, 2021

lucab commented Jun 1, 2021

dustymabe commented Jun 1, 2021

kelvinfan001 commented Jun 1, 2021 • edited Loading

kai-uwe-rommel commented Jun 5, 2021

kelvinfan001 commented Jun 30, 2021

dustymabe commented Jul 21, 2021

mysticaltech commented Jan 26, 2022

dustymabe commented Mar 20, 2023

jlebon commented Mar 20, 2023

strategy: add new `marker_file` strategy #540

strategy: add new `marker_file` strategy #540

kelvinfan001 commented Apr 28, 2021 •

edited

Loading

kelvinfan001 Apr 29, 2021 •

edited

Loading

kelvinfan001 commented Jun 1, 2021 •

edited

Loading