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

🐛 improve upper constraints handling #589

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

Rozzii
Copy link
Member

@Rozzii Rozzii commented Nov 6, 2024

This commit:

  • Provides a default value for the UPPER_CONSTRAINT docker file argument
  • Changes the prepare-image.sh logic to check the actual size of the upper constraint file and not the presence of it.

Background of this change is that it has been noticed that on release branches during image building the "pinned" upper constraint file is not picked up rather pulled from upstream OpenStack because of a lack of default argument value during image building.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2024
@Rozzii Rozzii changed the title 🐛 fix upper constraints release-24.0 🐛 improve upper constraints release-24.0 Nov 6, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Nov 6, 2024

/cc @elfosardo

@elfosardo
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elfosardo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2024
@tuminoid
Copy link
Member

tuminoid commented Nov 6, 2024

For reference: this is manual cherry-pick of #584

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2024
@tuminoid
Copy link
Member

tuminoid commented Nov 6, 2024

/retitle 🐛 improve upper constraints handling

@metal3-io-bot metal3-io-bot changed the title 🐛 improve upper constraints release-24.0 🐛 improve upper constraints handling Nov 6, 2024
@tuminoid
Copy link
Member

tuminoid commented Nov 6, 2024

/retitle 🐛 improve upper constraints handling

@tuminoid
Copy link
Member

tuminoid commented Nov 6, 2024

/test metal3-centos-e2e-integration-test-release-1-6 metal3-ubuntu-e2e-integration-test-release-1-6

@tuminoid
Copy link
Member

tuminoid commented Nov 6, 2024

/retest

1 similar comment
@Rozzii
Copy link
Member Author

Rozzii commented Nov 7, 2024

/retest

@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

/test metal3-centos-e2e-integration-test-release-1-6 metal3-ubuntu-e2e-integration-test-release-1-6

@tuminoid
Copy link
Member

tuminoid commented Nov 8, 2024

I think the deps have rot already for this version:

[2024-11-08T09:06:46.846Z] INFO: pip is looking at multiple versions of <Python from Requires-Python> to determine which version is compatible with other requirements. This could take a while.
[2024-11-08T09:06:46.846Z] INFO: pip is looking at multiple versions of gunicorn to determine which version is compatible with other requirements. This could take a while.
[2024-11-08T09:06:46.846Z] INFO: pip is looking at multiple versions of crudini to determine which version is compatible with other requirements. This could take a while.
[2024-11-08T09:06:46.846Z] Collecting crudini
[2024-11-08T09:06:46.846Z]   Downloading crudini-0.9.4-py2.py3-none-any.whl (21 kB)
[2024-11-08T09:06:46.846Z] ERROR: Cannot install -r /tmp/ironic-source-list-final (line 5) and sushy because these package versions have conflicting dependencies.
[2024-11-08T09:06:46.846Z] 
[2024-11-08T09:06:46.846Z] The conflict is caused by:
[2024-11-08T09:06:46.846Z]     The user requested sushy
[2024-11-08T09:06:46.846Z]     ironic 24.0.1.dev10 depends on sushy>=4.7.0
[2024-11-08T09:06:46.846Z]     The user requested (constraint) sushy===4.5.2
[2024-11-08T09:06:46.846Z] 
[2024-11-08T09:06:46.846Z] To fix this you could try to:
[2024-11-08T09:06:46.846Z] 1. loosen the range of package versions you've specified
[2024-11-08T09:06:46.846Z] 2. remove package versions to allow pip attempt to solve the dependency conflict
[2024-11-08T09:06:46.846Z] 
[2024-11-08T09:06:46.846Z] ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies
[2024-11-08T09:06:49.219Z] Error: building at STEP "RUN prepare-image.sh &&   rm -f /bin/prepare-image.sh": while running runtime: exit status 1

@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

@elfosardo is it okay if I uplift the constraint ?

@tuminoid
Copy link
Member

tuminoid commented Nov 8, 2024

@elfosardo is it okay if I uplift the constraint ?

Can we do quick check if its already been uplifted in the upstream? It might've changed since the contsraints file was saved here.

@Rozzii Rozzii added this to the ironic-image - v24.0 milestone Nov 8, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

@tuminoid at least AFAIK in the upstream branch the requirement is clear https://opendev.org/openstack/ironic/src/branch/bugfix/24.0/requirements.txt#L49 so there is not much question about it IMO. It is not possible to build it under 4.7.

@Rozzii Rozzii force-pushed the cp-fix-constraint-24.0 branch from cfe86ab to 278638a Compare November 8, 2024 12:31
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

/test metal3-centos-e2e-integration-test-release-1-6 metal3-ubuntu-e2e-integration-test-release-1-6

@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

@tuminoid at least AFAIK in the upstream branch the requirement is clear https://opendev.org/openstack/ironic/src/branch/bugfix/24.0/requirements.txt#L49 so there is not much question about it IMO. It is not possible to build it under 4.7.

Now I have checked the original release tag too and even at that time it was 4.7 https://opendev.org/openstack/ironic/src/tag/24.0.0/requirements.txt so I think setting it lower was a mistake initially too just the bug served as a "workaround".

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Nov 8, 2024

Seems it will be multiple of the dependencies, I will push the fixes.

@tuminoid
Copy link
Member

/hold
More fixes needed.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2024
This commit:
 - Provides a default value for the UPPER_CONSTRAINT docker file argument
 - Changes the prepare-image.sh logic to check the actual size of the
   upper constraint file and not the presence of it.

Background of this change is that it has been noticed that on release branches
during image building the "pinned" upper constraint file is not picked up
rather pulled from upstream OpenStack because of a lack of default argument
value during image building.

Signed-off-by: Adam Rozman <[email protected]>
@Rozzii Rozzii force-pushed the cp-fix-constraint-24.0 branch from 278638a to 3c611b1 Compare December 4, 2024 07:17
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Dec 4, 2024

Should be fine now.
/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2024
@Rozzii
Copy link
Member Author

Rozzii commented Dec 4, 2024

/test metal3-centos-e2e-integration-test-release-1-6 metal3-ubuntu-e2e-integration-test-release-1-6

@elfosardo
Copy link
Member

@Rozzii sorry for ignoring this! It got completely out of my radar after I approved it.
my understanding is that the initial UC pinning was correct, is that right?
Also please let me know if you need help troubleshooting

@Rozzii
Copy link
Member Author

Rozzii commented Dec 5, 2024

/test metal3-centos-e2e-integration-test-release-1-6 metal3-ubuntu-e2e-integration-test-release-1-6

@Rozzii
Copy link
Member Author

Rozzii commented Dec 9, 2024

@Rozzii sorry for ignoring this! It got completely out of my radar after I approved it. my understanding is that the initial UC pinning was correct, is that right? Also please let me know if you need help troubleshooting

AFAIK originally this has worked, I assume the change of the UC pinning is related to 24.1 release.

@Rozzii Rozzii requested a review from tuminoid December 11, 2024 14:08
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Good job finding working combination!

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2024
@metal3-io-bot metal3-io-bot merged commit 78537b2 into metal3-io:release-24.0 Dec 11, 2024
8 checks passed
@metal3-io-bot metal3-io-bot deleted the cp-fix-constraint-24.0 branch December 11, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Status: Ironic-image WIP
Development

Successfully merging this pull request may close these issues.

4 participants