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.

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

Jump to bottom

Update lxd_container with new API endpoint #7854

Closed
wants to merge 2 commits into from
Closed

Update lxd_container with new API endpoint #7854

wants to merge 2 commits into from

Conversation

johnmaguire
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Jan 16, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 17, 2024
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 17, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 Automatically create a backport for the stable-7 branch backport-8 Automatically create a backport for the stable-8 branch labels Jan 17, 2024
felixfontein
felixfontein reviewed Jan 17, 2024
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please add a changelog fragment. Thanks.

plugins/modules/lxd_container.py
self.api_endpoint = '/1.0/containers'
elif self.type == 'virtual-machine':
self.api_endpoint = '/1.0/virtual-machines'
self.api_endpoint = '/1.0/instances'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is likely a breaking change for older LXD versions.

Copy link
Author

Choose a reason for hiding this comment

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

LXD has supported this endpoint since v4.0 (LTS) released April 3, 2020: https://github.com/canonical/lxd/blob/lxd-4.0.0/doc/api-extensions.md (announcement)

I'm not sure what backwards compatibility guarantees community.general makes or how best to preserve backwards compatibility with older versions. Can you provide guidance?

Copy link
Author

Choose a reason for hiding this comment

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

LXD v3.0 LTS branch reached EOL in July 2023: https://discuss.linuxcontainers.org/t/lxc-lxd-lxcfs-3-0-end-of-life-announcement/17600

Copy link
Collaborator

Choose a reason for hiding this comment

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

The module unfortunately does not specify an explicit lower bound of the LXD versions it supports.

The best way would be to have some version detection and then a version based behavior selection (grep for LooseVersion in this repo to see how version comparisons can be done). I would guess there is an API endpoint which allows you to get info on LXD itself, including its version, that can be used for this.

Copy link
Contributor

@anthonyra anthonyra Feb 6, 2024 •
edited

Choose a reason for hiding this comment

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

It's currently breaking for pure LXD and not just Incus. With LXD version 5.19

plugins/modules/lxd_container.py
@@ -547,6 +543,8 @@ def _create_instance(self):
url_params['target'] = self.target
if self.project:
url_params['project'] = self.project
if self.type:
url_params['type'] = self.type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this also break older LXD versions?

Copy link
Author

Choose a reason for hiding this comment

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

I have not explicitly tested it, but a quick peruse of the code suggests that unexpected query parameters are ignored: https://github.com/canonical/lxd/blob/cf9bf2ac072044118dfbd9d49f2342298a52c2d2/lxd/instances_get.go#L244

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be good.

@johnmaguire
Copy link
Author

CI is failing on files unrelated to this PR - I'm unsure how to resolve.

@felixfontein
Copy link
Collaborator

The CI failure is unrelated, I fixed it in #7858 - it should go away on the next CI run.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 26, 2024
@antifob
Copy link

antifob commented Feb 16, 2024

Hi, I see #7858 has been merged. Is it possible to run the CI on this PR in order to get it merged?

Regards

@johnmaguire
Copy link
Author

@antifob It sounds like the maintainers are looking for additional changes to support EOL'd versions of LXD, which I'm not in a position to test. I suggest if someone has the time to do so, they create a new PR.

@antifob
Copy link

antifob commented Feb 16, 2024

I understand the concern, but the currently-used API endpoints no longer exist so this module.

@felixfontein It seems this module is currently broken and can't move forward because of deprecation concerns. Considering that it's the choice of users of deprecated LXD versions (if they even exist) to stay there, would adding a new endpoint option be appropriate? It could default to the current endpoint and allow those users to force an old endpoint they could use.

@felixfontein
Copy link
Collaborator

@antifob I very well understand that the module is currently broken, but fixing it by breaking it for other users for which it currently works is not the solution. Please check out our guidelines: #582 (comment) / https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_requirements.html#collections-requirements

Adding an endpoint option is possible, but sounds like a very clumsy workaround. I'm pretty sure the API allows to query the version of LXD somehow. Using that it should be very simple to fix this: query the version, and then choose the endpoint depending on the version.

@anthonyra
Copy link
Contributor

anthonyra commented Feb 17, 2024 •
edited

My attempt at fixing this issue #7980 it also fixes the placement of where type needs to go on the new API endpoint call.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

@anthonyra anthonyra anthonyra left review comments

Assignees
No one assigned
Labels
backport-7 Automatically create a backport for the stable-7 branch backport-8 Automatically create a backport for the stable-8 branch bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Incus in lxd_container module
5 participants
@johnmaguire @ansibullbot @felixfontein @antifob @anthonyra