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

Add amphora image tag configuration #383

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Add amphora image tag configuration #383

merged 4 commits into from
Nov 19, 2024
+33 −18

Conversation

weinimo
Copy link
Collaborator

@weinimo weinimo commented Sep 24, 2024 •
edited
Loading

This configures the cloud to use the amphora
image with vertical scaling optimization when
using a compute flavor with multiple vCPUs.

https://issues.redhat.com/browse/OSPRH-8446

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 24, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: weinimo

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

@weinimo weinimo marked this pull request as ready for review September 24, 2024 09:30
@openshift-ci openshift-ci bot requested review from olliewalsh and viroel September 24, 2024 09:30
gthiemonge
gthiemonge reviewed Sep 30, 2024
View reviewed changes
pkg/amphoracontrollers/flavors.go Outdated
@@ -230,7 +238,7 @@ func ensureFlavors(osclient *openstack.OpenStack, log *logr.Logger, instance *oc
log.Info(fmt.Sprintf("Creating Octavia flavor profile \"%s\"", flavorProfileCreateOpts.Name))
fp, err := flavorprofiles.Create(lbClient, flavorProfileCreateOpts).Extract()
if err != nil {
return "", err
return "", fmt.Errorf("error creating flavor profiles: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to have a fallback when a flavorprofile cannot be created (for instance if the image tag doesn't exist).
we could ignore the error and continue, or maybe report an error only if none of the flavors were created successfully.

Right now, this patch will block the promotion pipeline with upstream content

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've pushed an update that should implement that idea.

@weinimo weinimo force-pushed the amphora-image-tag branch from 529f783 to c3b4d38 Compare October 1, 2024 11:24
gthiemonge
gthiemonge reviewed Oct 9, 2024
View reviewed changes
pkg/amphoracontrollers/flavors.go Outdated
flavorSuccess = true
}
if !flavorSuccess {
return "", fmt.Errorf("none of the Octavia flavors could be configured because of errors. last error: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't seem to work as expected:

error while creating flavors: none of the Octavia flavors could be configured because of errors. last error: %!w(<nil>)

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with this PR is that it floods the output with errors and backtraces until an image is uploaded.
A solution would be to create the amphora flavor profile without amp_image_tag, it would pass and use the default tag from octavia.conf

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try that. Also I set ctrl.Result{RequeueAfter: time.Duration(180) * time.Second}, nil to avoid flooding the output with errors should they occur.

pkg/amphoracontrollers/flavors.go Outdated
@@ -230,7 +239,12 @@ func ensureFlavors(osclient *openstack.OpenStack, log *logr.Logger, instance *oc
log.Info(fmt.Sprintf("Creating Octavia flavor profile \"%s\"", flavorProfileCreateOpts.Name))
fp, err := flavorprofiles.Create(lbClient, flavorProfileCreateOpts).Extract()
if err != nil {
return "", err
errFmt := fmt.Errorf("error creating flavor profile: %w", err)
log.Error(errFmt, fmt.Sprintf("Amphora image might be missing or not "+
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a log.Warning would be more appropriate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

log.Logger does not seem to have a warning level. I change this to Info. https://pkg.go.dev/github.com/go-logr/[email protected]#hdr-Usage

@beagles
Copy link
Collaborator

beagles commented Oct 18, 2024

Added hold tag label as we are soft-frozen except for blockers/critical for the time being.

@weinimo
Copy link
Collaborator Author

weinimo commented Nov 6, 2024

/retest
error: build error: Failed to push image: trying to reuse blob sha256:4d5d1cbd7ece41ce278c26338e01e2b82e1861b820ca052da9f3e0b16815358f at destination: pinging container registry quay.rdoproject.org: Get "https://quay.rdoproject.org/v2/": dial tcp 38.129.56.158:443: i/o timeout

@weinimo
Copy link
Collaborator Author

weinimo commented Nov 6, 2024

/retest different error this time

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 6, 2024

@weinimo: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test functional
  • /test images
  • /test octavia-operator-build-deploy-kuttl
  • /test precommit-check

The following commands are available to trigger optional jobs:

  • /test octavia-operator-build-deploy

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openstack-k8s-operators-octavia-operator-main-functional
  • pull-ci-openstack-k8s-operators-octavia-operator-main-images
  • pull-ci-openstack-k8s-operators-octavia-operator-main-octavia-operator-build-deploy-kuttl
  • pull-ci-openstack-k8s-operators-octavia-operator-main-precommit-check

In response to this:

/retest different error this time

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@weinimo
Copy link
Collaborator Author

weinimo commented Nov 6, 2024

/retest

@weinimo
Copy link
Collaborator Author

weinimo commented Nov 11, 2024

/retest
error: must build at directory: not a valid directory: evalsymlink failure on '/alabama/install_yamls/out/octavia-kuttl-tests/nova/cr' : lstat /alabama/install_yamls/out/octavia-kuttl-tests/nova: no such file or directory

@fernandoroyosanchez
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 18, 2024
@weinimo
Copy link
Collaborator Author

weinimo commented Nov 19, 2024

/retest
error seems unrelated

@weinimo
Copy link
Collaborator Author

weinimo commented Nov 19, 2024

/retest
could not find the error in the logs

@weinimo
Add amphora image tag configuration
8c55458 
This configures the cloud to use the amphora
image with vertical scaling optimization when
using a compute flavor with multiple vCPUs.

JIRA: https://issues.redhat.com/browse/OSPRH-8446
@weinimo
Add some context to error object in flavors.go
84fcfdd
@weinimo
Skip flavor configuration on error
aa9e3f0 
Return error only if none of the flavors could
be configured.
@weinimo
Skip flavor configuration on error
5db75f1 
Return error only if none of the flavors could
be configured.
@openshift-ci openshift-ci bot removed the lgtm label Nov 19, 2024
@fernandoroyosanchez
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 19, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e75b9af into main Nov 19, 2024
7 of 8 checks passed
@openshift-merge-bot openshift-merge-bot bot deleted the amphora-image-tag branch November 19, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gthiemonge gthiemonge gthiemonge left review comments

@olliewalsh olliewalsh Awaiting requested review from olliewalsh

@viroel viroel Awaiting requested review from viroel

@beagles beagles Awaiting requested review from beagles

Assignees

@fernandoroyosanchez fernandoroyosanchez

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weinimo @beagles @fernandoroyosanchez @gthiemonge