Skip to content

cephadm: Modify the structure of the default container images#60412

Merged
adk3798 merged 1 commit intoceph:mainfrom
ShwetaBhosale1:fix_issue_68605_modify_structure_of_default_images
Dec 2, 2024
Merged

cephadm: Modify the structure of the default container images#60412
adk3798 merged 1 commit intoceph:mainfrom
ShwetaBhosale1:fix_issue_68605_modify_structure_of_default_images

Conversation

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor

@ShwetaBhosale1 ShwetaBhosale1 commented Oct 21, 2024

cephadm: Modify the structure of the default container images

Fixes: https://tracker.ceph.com/issues/68605

Signed-off-by: Shweta Bhosale Shweta.Bhosale1@ibm.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

This PR has cherry-picked commit of PR: #60377.

@adk3798 adk3798 changed the title Fix issue 68605 modify structure of default images cephadm: Modify the structure of the default container images Oct 21, 2024
@adk3798
Copy link
Copy Markdown
Contributor

adk3798 commented Oct 21, 2024

I updated the PR title. It's not required to include the tracker being fixed in the PR title and having the component being updated in the title helps identify what's being changed without having to look through the changes.

@github-actions
Copy link
Copy Markdown

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@ShwetaBhosale1 ShwetaBhosale1 force-pushed the fix_issue_68605_modify_structure_of_default_images branch from 1e9a9c7 to 01b01f8 Compare November 4, 2024 04:08
@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test api

Copy link
Copy Markdown
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

What's here looks pretty good and works. Added a comment about further centralizing the config names on top of the images.

list-images
-----------

List the default container images for all services
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.

We might want to add to this description that we list them in an ini format so the results can be modified and passed to the cluster with the --config flag during bootstrap.

@ShwetaBhosale1 ShwetaBhosale1 force-pushed the fix_issue_68605_modify_structure_of_default_images branch 3 times, most recently from 7da4a4f to d1cb1b7 Compare November 6, 2024 08:29
Copy link
Copy Markdown
Contributor

@rkachach rkachach left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Very good initiative @ShwetaBhosale1 :)

.. just some comments to improve the code readability/maintenance and to boost the docs automation.

Copy link
Copy Markdown
Contributor

@rkachach rkachach left a comment

Choose a reason for hiding this comment

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

This is a small detail but I think it's very important to name the field in a descriptive way.

Copy link
Copy Markdown
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

Outside of comments Redouane has already provided, this LGTM.

@ShwetaBhosale1 ShwetaBhosale1 force-pushed the fix_issue_68605_modify_structure_of_default_images branch from d1cb1b7 to 1cb695b Compare November 7, 2024 14:38
Copy link
Copy Markdown
Contributor

@rkachach rkachach left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I have one small nitpick that you can either follow up on in this PR or in a future one.

One other suggestion is to have the doc changes be a separate commit to the code change. It makes things easier on the reviewer and future explorers of the code.

),
]
for image in DefaultImages:
MODULE_OPTIONS.append(Option(image.key, default=image.image_ref, desc=image.desc))
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.

:-D

return self.image_ref


def create_image(image_ref: str, key: str) -> ContainerImage:
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.

Tiny nitpick: if this function is not intended to be used outside of this file name it: _create_image. That way you can change it freely without worrying if code has imported and used this function elsewhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with function name change.

@ShwetaBhosale1 ShwetaBhosale1 force-pushed the fix_issue_68605_modify_structure_of_default_images branch from 26424df to 6b57877 Compare November 12, 2024 04:59
@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test api

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test dashboard cephadm

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test api

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test dashboard cephadm

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test api

@adk3798
Copy link
Copy Markdown
Contributor

adk3798 commented Nov 21, 2024

https://pulpito.ceph.com/adking-2024-11-19_02:02:13-orch:cephadm-wip-adk-testing-2024-11-18-1758-distro-default-smithi/

Failures:

  • lots of upgrade failures, primarily because the dashboard cherrypy backports issue fix was still not present in reef when these tests ran
  • 1 MON_DOWN cluster log warning, seen this before, need to update the ignoreslist for this test
  • 1 selinux denial failure in an smb ctdb test, known issue
  • 2 new mgmt-gateway tests being added by a PR in the run failed
  • 1 strange failure applying an SMB spec. Seems like it could be a bug that is just rarely hit in that test, as no SMB related PRs were included in the run

I wouldn't merge any SMB changes or the new tests off of this run, but otherwise it's okay.

@github-actions
Copy link
Copy Markdown

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@ShwetaBhosale1 ShwetaBhosale1 force-pushed the fix_issue_68605_modify_structure_of_default_images branch from 6b57877 to 77605ce Compare November 22, 2024 05:03
@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

Resolved merge conflict of nvmeof.py file

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test submodules

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

3 similar comments
@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

3 similar comments
@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

@ShwetaBhosale1
Copy link
Copy Markdown
Contributor Author

jenkins test make check arm64

@adk3798 adk3798 merged commit eb7c4f5 into ceph:main Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants