-
Notifications
You must be signed in to change notification settings - Fork 27
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
Create Nova flavors for Octavia #200
Create Nova flavors for Octavia #200
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gthiemonge 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 |
pkg/amphoracontrollers/flavors.go
Outdated
Name: "octavia-amphora", | ||
VCPUs: 1, | ||
RAM: 1024, | ||
Disk: gophercloud.IntToPointer(3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you pointed out in a discussion downsteam already, 3 GB might not be enough. I guess we can leave it like this for now, but it would probably be good to add at least a TODO comment to verify this is enough, or maybe we can make this configurable between a certain range >3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the other solution would be to enable log offloading and disable local disk storage by default, in that case 3GB would be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm in general
} | ||
r.Log.Info(fmt.Sprintf("Using default flavor \"%s\"", defaultFlavorID)) | ||
|
||
err = r.generateServiceConfigMaps(ctx, instance, helper, &configMapVars, networkID, defaultFlavorID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we should create a new struct that contains networkID
, defaultFlavorID
and potentally more things and use that instead here in order to keep the list of arguments short.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I can do that
} | ||
|
||
isPublic := false | ||
defaultFlavorsCreateOpts := []flavors.CreateOpts{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the name is pretty much self explanatory we should probably add a Description
to each flavor as well. The description of the first flavor should mention that this is the default flavor.
Add support for system-scope Compute client. Ensure that 2 nova flavors are created: - octavia-amphora: 1 vCPU, 1GB RAM, 3GB disk - octavia-amphora-4vcpus: 4 vCPU, 4GB RAM, 3GB disk Flavors in previous releases used non-UUIDs as ID (octavia_65, etc...) and didn't have a clear naming convention (amphora*, octavia*). Don't force the ID of the new flavors, let Nova generates them. octavia-amphora is the default flavor in Octavia
/lgtm |
37c8c91
into
openstack-k8s-operators:main
Add support for system-scope Compute client.
Ensure that 2 nova flavors are created:
Flavors in previous releases used non-UUIDs as ID (octavia_65, etc...) and didn't have a clear naming convention (amphora*, octavia*). Don't force the ID of the new flavors, let Nova generates them.
octavia-amphora is the default flavor in Octavia