Skip to content
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

rework handling of local storage #247

Merged
merged 9 commits into from
Sep 18, 2019
Merged

rework handling of local storage #247

merged 9 commits into from
Sep 18, 2019

Conversation

bcressey
Copy link
Contributor

Issue #, if available:
#167

Description of changes:
We now reserve /var and /opt as persistent storage for customer use. /var/lib/thar continues to hold persistent files for the OS, but other users of /var have been relocated to /run.

Testing done

  • glibc and chrony use their new paths for cache and state files.
  • systemd uses the new paths for chrony (and a test service).
  • /local, /var, and /opt are mounted at boot.
  • /opt/cni/bin is populated at boot (and later modified by the EKS CNI plugin).
  • docker, kubelet, and containerd use paths under /var/lib.
  • amiize can register an AMI with a separate data volume.
  • The data volume is resized to fill the partition when the AMI boots.
  • The instance is able to connect to the EKS cluster.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This also simplifies the packaging by relying on more features of tmpfiles.

Signed-off-by: Ben Cressey <[email protected]>
@bcressey bcressey requested review from iliana and jamieand September 18, 2019 15:37
let unix_epoch = FileTime::zero();

// Set the file modification times for /etc, /var, /opt to the unix epoch time to ensure that
// Set the file modification times for /etc to the unix epoch time to ensure that
// systemd detect these directories as 'outdated/uninitialized' and perform all the initialization
Copy link
Contributor

Choose a reason for hiding this comment

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

"detects this directory as ... and performs"


case SD_PATH_SYSTEM_STATE_LOGS:
- *ret = "/var/log";
+ *ret = "/run/log";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we lose the journal on reboot? This is probably not ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not ideal but it's also not new behavior.

We'd also need to persist the machine ID for a durable journal to be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#250 for persistent journal

@@ -1,7 +1,7 @@
{{#each settings.ntp.time-servers}}
pool {{this}} iburst
{{/each}}
driftfile /var/lib/thar/chrony/drift
driftfile /run/lib/chrony/drift
Copy link
Contributor

Choose a reason for hiding this comment

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

The drift file shouldn't be ephemeral, especially if we are often rebooting to apply updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and also the lack of persistence for DHCP leases is a concern.

We need to make sure both wicked and chrony are hardened against corrupted input before we allow them to read from persistent storage.

Copy link
Contributor

@iliana iliana Sep 18, 2019

Choose a reason for hiding this comment

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

If we want to merge this as-is, I'll ask for an issue tracking persistence of the drift file and DHCP leases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#248 for wicked and #249 for chrony.

bin/rpm2img Outdated
# boot: 20M + root: 900M + hash: 8M + reserved: 95M = 1023M
# boot partition attributes (-A): 48 = gptprio priority bit; 56 = gptprio successful bit
# partitions are backwards so that we don't make things inconsistent when specifying a wrong end sector :)
sgdisk --clear \
-n 0:2048M:4095M -c 0:"THAR-DATA" -t 0:8300 \
-n 0:1953M:0 -c 0:"THAR-RESERVED-B" -t 0:"${THAR_RESERVED_TYPECODE}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-n 0:1953M:0 -c 0:"THAR-RESERVED-B" -t 0:"${THAR_RESERVED_TYPECODE}" \
-n 0:1953M:2047M -c 0:"THAR-RESERVED-B" -t 0:"${THAR_RESERVED_TYPECODE}" \

packages/containerd/containerd-tmpfiles.conf Show resolved Hide resolved
This moves the THAR-DATA mount from /var/lib/thar to /local, where
we create the directories necessary to mount /var and /opt from
that volume.

Since the directories are persistent, we no longer need to create
the correspoindg tmpfs or overlayfs mounts during preinit.

Both mounts are now wanted by the local-fs.target, so most units do
not need to declare an explicit dependency.

Signed-off-by: Ben Cressey <[email protected]>
This replaces the previous overlayfs method, and still allows for
containers launched by the orchestrator agent to add or replace
plugins.

Signed-off-by: Ben Cressey <[email protected]>
Although these services ship as part of the OS, we want to enable
customers to replace the persistent storage volume with their own
copy, for example to supply a cache of downloaded container images.

Paths such as `/var/lib/docker` make it easier for these volumes
to be created without baking in a lot of OS specific details.

Signed-off-by: Ben Cressey <[email protected]>
@bcressey bcressey merged commit 33b43a6 into develop Sep 18, 2019
@bcressey bcressey deleted the local-storage branch September 18, 2019 19:23
@bcressey bcressey mentioned this pull request Sep 18, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants