mgr/cephadm: convert tags to repo_digest#36432
Merged
sebastian-philipp merged 5 commits intoceph:masterfrom Sep 7, 2020
Merged
mgr/cephadm: convert tags to repo_digest#36432sebastian-philipp merged 5 commits intoceph:masterfrom
sebastian-philipp merged 5 commits intoceph:masterfrom
Conversation
3 tasks
varshar16
reviewed
Aug 4, 2020
Contributor
|
Please update vstart too. |
Contributor
Author
wdyt of the overall approach? |
0ea7c35 to
2e96e8a
Compare
3 tasks
Contributor
It looks good. |
varshar16
suggested changes
Aug 10, 2020
Contributor
varshar16
left a comment
There was a problem hiding this comment.
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1092, in _remote_connection
yield (conn, connr)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1135, in _run_cephadm
image = self._get_container_image(entity)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1066, in _get_container_image
assert False, daemon_type
AssertionError: client
ERROR orchestrator._interface:_interface.py:346 _Promise failed
Traceback (most recent call last):
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/orchestrator/_interface.py", line 299, in _finalize
next_result = self._on_complete(self._value)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 102, in <lambda>
return CephadmCompletion(on_complete=lambda _: f(*args, **kwargs))
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1229, in add_host
return self._add_host(spec)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1215, in _add_host
error_ok=True, no_fsid=True)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1135, in _run_cephadm
image = self._get_container_image(entity)
File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/cephadm/module.py", line 1066, in _get_container_image
assert False, daemon_type
AssertionError: client
2e96e8a to
45d05f8
Compare
3 tasks
6c7f174 to
e6de0f3
Compare
Contributor
Author
Hm. turns out: we have different goals here:
I can see your point, but I think this would be a follow-up PR. |
Contributor
Please update the doc too either in this PR or follow-up. The |
varshar16
suggested changes
Aug 31, 2020
Contributor
varshar16
left a comment
There was a problem hiding this comment.
The test actually did not work if use_repo_digest is set:
2020-08-28T19:32:47.906 INFO:teuthology.orchestra.run.smithi051.stdout:NAME HOST STATUS REFRESHED AGE VERSION IMAGE NAME IMAGE ID CONTAINER ID
2020-08-28T19:32:47.907 INFO:teuthology.orchestra.run.smithi051.stdout:alertmanager.a smithi051 running (3m) 3s ago 6m 0.21.0 docker.io/prom/alertmanager:latest c876f5897d7b 702b3f51c5fe
2020-08-28T19:32:47.907 INFO:teuthology.orchestra.run.smithi051.stdout:grafana.a smithi038 running (5m) 1s ago 5m 6.6.2 docker.io/ceph/ceph-grafana:latest 87a51ecf0b1c 76b91cae48d6
2020-08-28T19:32:47.907 INFO:teuthology.orchestra.run.smithi051.stdout:mgr.x smithi038 running (3m) 1s ago 8m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 97b8237794b0
2020-08-28T19:32:47.907 INFO:teuthology.orchestra.run.smithi051.stdout:mgr.y smithi051 running (104s) 3s ago 10m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 f5285b655a81
2020-08-28T19:32:47.908 INFO:teuthology.orchestra.run.smithi051.stdout:mon.a smithi051 running (2m) 3s ago 10m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 cdf5ec15899e
2020-08-28T19:32:47.908 INFO:teuthology.orchestra.run.smithi051.stdout:mon.b smithi038 running (2m) 1s ago 9m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 8211285b9228
2020-08-28T19:32:47.908 INFO:teuthology.orchestra.run.smithi051.stdout:mon.c smithi051 running (2m) 3s ago 9m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 2d6be1cf5511
2020-08-28T19:32:47.908 INFO:teuthology.orchestra.run.smithi051.stdout:node-exporter.a smithi051 running (6m) 3s ago 6m 1.0.1 docker.io/prom/node-exporter:latest 0e0218889c33 cd0c8c8ad842
2020-08-28T19:32:47.909 INFO:teuthology.orchestra.run.smithi051.stdout:node-exporter.b smithi038 running (6m) 1s ago 6m 1.0.1 docker.io/prom/node-exporter:latest 0e0218889c33 5651378ef705
2020-08-28T19:32:47.909 INFO:teuthology.orchestra.run.smithi051.stdout:osd.0 smithi051 running (30s) 3s ago 8m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 c2fd2ae5820b
2020-08-28T19:32:47.909 INFO:teuthology.orchestra.run.smithi051.stdout:osd.1 smithi051 running (20s) 3s ago 8m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 2060c8529ce6
2020-08-28T19:32:47.909 INFO:teuthology.orchestra.run.smithi051.stdout:osd.2 smithi051 running (12s) 3s ago 8m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 fdb420ad7983
2020-08-28T19:32:47.909 INFO:teuthology.orchestra.run.smithi051.stdout:osd.3 smithi051 running (4s) 3s ago 7m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 5c3e6ac126fd
2020-08-28T19:32:47.910 INFO:teuthology.orchestra.run.smithi051.stdout:osd.4 smithi038 running (96s) 1s ago 7m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 e2a4d5dc9712
2020-08-28T19:32:47.910 INFO:teuthology.orchestra.run.smithi051.stdout:osd.5 smithi038 running (86s) 1s ago 7m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 5d522fa4fbc7
2020-08-28T19:32:47.910 INFO:teuthology.orchestra.run.smithi051.stdout:osd.6 smithi038 running (76s) 1s ago 7m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 86ad28ae25b8
2020-08-28T19:32:47.910 INFO:teuthology.orchestra.run.smithi051.stdout:osd.7 smithi038 running (53s) 1s ago 6m 16.0.0-4841-gddc6595 quay.ceph.io/ceph-ci/ceph:ddc65956ac220ef29f86671e4c9a8bcbce4d500f 515273432f01 8f023675de2b
2020-08-28T19:32:47.910 INFO:teuthology.orchestra.run.smithi051.stdout:prometheus.a smithi038 running (5m) 1s ago 6m 2.20.1 docker.io/prom/prometheus:latest b205ccdd28d3 8ac16cf9358a
Contributor
Author
I do know for sure that the global |
Contributor
I don't think the error is due to this PR. I get the same result even on master branch. Before setting After setting |
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
As this is the most interesting test suite Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
* Automatically convert tags like `:latest` to the digest * Use the digest instead of the tag Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
4b12fce to
303bbdf
Compare
Contributor
Author
|
rebased |
smithfarm
reviewed
Sep 3, 2020
| 'name': 'use_repo_digest', | ||
| 'type': 'bool', | ||
| 'default': False, | ||
| 'desc': 'Automatically convert image tags to image digest. Make sure all daemons use the same image', |
Contributor
There was a problem hiding this comment.
The description sounds like the behavior users would expect to be active by default, so I'm confused why the default is set to "False".
Contributor
Author
There was a problem hiding this comment.
Eh, the description describes the flag itself. Independent of the value.
Member
There was a problem hiding this comment.
+1 to use true as default value. Is the safe option!
3 tasks
Contributor
Author
|
according to The teuthology log looks like so: Not that mgr.y is shown as using Which means, the container image is actually correct! @varshar16 are you ok with merging this now? |
Contributor
Author
jmolmo
reviewed
Sep 4, 2020
|
|
||
| def get_image_info_from_inspect(out, image): | ||
| # type: (str, str) -> Dict[str, str] | ||
| image_id, digests = out.split(',', 1) |
Member
There was a problem hiding this comment.
maybe more coherent if you move this before you check if out is empty
jmolmo
reviewed
Sep 4, 2020
| cd.command_registry_login() | ||
| assert str(e.value) == "Failed to login to custom registry @ sample-url as sample-user with given password" | ||
|
|
||
| def test_get_image_info_from_inspect(self): |
Member
There was a problem hiding this comment.
What about to test it with different combinations of "image id" and "repo_digest" empty?
jmolmo
reviewed
Sep 4, 2020
| self.registry_url: Optional[str] = None | ||
| self.registry_username: Optional[str] = None | ||
| self.registry_password: Optional[str] = None | ||
| self.use_repo_digest = False |
Member
There was a problem hiding this comment.
I insist. True by default seems more safe for everybody
jmolmo
reviewed
Sep 4, 2020
| self.log.debug(f'image {image_name} -> {r}') | ||
| return r | ||
| except (ValueError, KeyError) as _: | ||
| msg = 'Failed to pull %s on %s: %s' % (image_name, host, '\n'.join(out)) |
Member
There was a problem hiding this comment.
You haven't failed pulling the image, you have failed trying to process the output of the command
jmolmo
approved these changes
Sep 4, 2020
Member
jmolmo
left a comment
There was a problem hiding this comment.
Just Minor nits... and this is something that can avoid "weird" problems!!!
mgfritch
approved these changes
Sep 4, 2020
Comment on lines
+536
to
+553
| def convert_tags_to_repo_digest(self): | ||
| if not self.use_repo_digest: | ||
| return | ||
| settings = self.upgrade.get_distinct_container_image_settings() | ||
| digests: Dict[str, ContainerInspectInfo] = {} | ||
| for container_image_ref in set(settings.values()): | ||
| if not is_repo_digest(container_image_ref): | ||
| image_info = self._get_container_image_info(container_image_ref) | ||
| if image_info.repo_digest: | ||
| assert is_repo_digest(image_info.repo_digest), image_info | ||
| digests[container_image_ref] = image_info | ||
|
|
||
| for entity, container_image_ref in settings.items(): | ||
| if not is_repo_digest(container_image_ref): | ||
| image_info = digests[container_image_ref] | ||
| if image_info.repo_digest: | ||
| self.set_container_image(entity, image_info.repo_digest) | ||
|
|
Contributor
There was a problem hiding this comment.
minor nit: this is difficult to read, maybe a few comments about what each loop does would help ..
basically we are building a list of digests and only setting the container image iff the config value is a label (and not an existing digest).
|
|
||
| try: | ||
|
|
||
| self.convert_tags_to_repo_digest() |
Contributor
There was a problem hiding this comment.
nit: a comment here that the digest only changes iff the container was set to ref the image by label
having this routine in the serve() thread implies that the digest is always changes which is not how it's actually implemented
| for opt in config: | ||
| if opt['name'] == 'container_image': | ||
| image_settings[opt['section']] = opt['value'] | ||
| image_settings = self.get_distinct_container_image_settings() |
Contributor
There was a problem hiding this comment.
upgrade always converts from the supplied image to a new digest 👍
Contributor
ceph orch ps is technically correct: the image is correct. It's just shown as not using the repo digest |
varshar16
approved these changes
Sep 7, 2020
Contributor
varshar16
left a comment
There was a problem hiding this comment.
Requires follow-up pr to address multiple issues found and test needs to be modified to check if images use repo digest.
Contributor
Author
|
follow up issue: https://tracker.ceph.com/issues/47332 |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blocked by
ceph orch redeploy <what>#36426This PR allows you to run
And cephadm will then implicitly convert
:latestto the recent sha256 digest and us the digest instead.Or, it provides a way to convert the global container_image config to the digest:
TODO
Follow-up PRs / known issues:
podman psshows the tag name instead of the digest which is irritatingChecklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox