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

Operator 0.5.3 #215

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Operator 0.5.3 #215

merged 3 commits into from
Jan 25, 2022

Conversation

jmesnil
Copy link
Member

@jmesnil jmesnil commented Jan 17, 2022

jmesnil and others added 2 commits January 17, 2022 10:26
Signed-off-by: Jeff Mesnil <[email protected]>
* added resources directive required for autoscaling
* added selector label for CR required by the HPA
  the unique name label should suffice as the selector label

This fixes wildfly#211
@jmesnil jmesnil marked this pull request as ready for review January 19, 2022 12:17
@jmesnil jmesnil added this to the 0.5.3 milestone Jan 19, 2022
@jmesnil jmesnil added Auto Pilot (V) Features for Phase V of the Operator capability levels Basic Install (I) Features for Phase I of the Operator capability levels enhancement New feature or request labels Jan 19, 2022
@jmesnil jmesnil linked an issue Jan 19, 2022 that may be closed by this pull request
@jmesnil jmesnil requested a review from yersan January 19, 2022 12:52
@jmesnil
Copy link
Member Author

jmesnil commented Jan 19, 2022

@yersan could you please review and approve?
I've aggregated the PR for #210 & #212 and squash them to get a single PR that contains all that is needed for HPA.

Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

@jmesnil found a minor harmless detail. Besides that, looks good to me

deploy/crds/wildfly.org_wildflyservers_crd.yaml Outdated Show resolved Hide resolved
* Update doc
* Use same code for statefulset and pvc resources

this fixes wildfly#211
[[resources]]
## Specify the Resource requirements for the container

The `resources` spec is defined in the link:../apis.adoc#Resources[Resources API Documentation].
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for reporting it @mchoma , I've just opened #217 to address this one and the others, both documents have broken links at several places.

@@ -44,16 +44,31 @@ It uses a `StatefulSet` with a pod spec that mounts the volume specified by `sto
| `bootableJar` | BootableJar specifies whether the application image is using WildFly S2I Builder/Runtime images or Bootable Jar. If omitted,
it defaults to false (application image is expected to use WildFly S2I Builder/Runtime images) | bool | false
| `standaloneConfigMap` | spec to specify how standalone configuration can be read from a `ConfigMap` | *<<standaloneconfigmapspec>> |false
| `resources`| Resources spec to specify the request or limits of the Stateful Set. If ommited, the namespace defaults are used. | *<<resources>> | false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask? From how this is documented here user can think there should be resources.resurces field. But in example there is just one field resources.

Shouldn't be here resources directly of type https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#resourcerequirements-v1-core[corev1.ResourceRequirements ? Analogous as env or envFrom?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

@yersan yersan Feb 11, 2022

Choose a reason for hiding this comment

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

Hi @mchoma,

I understand your concern, the key point here is that the value at the Scheme column is a pointer to another structure (notice it starts with *, and not a structure itself. So, resources is like *resources, which is indeed a https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#resourcerequirements-v1-core[corev1.ResourceRequirements

This specific case falls into these others:

It could be subtle for the user's thought, but after you run/check an example, you can realize it. Maybe having more quick examples could help with this.

Copy link
Collaborator

@yersan yersan Feb 11, 2022

Choose a reason for hiding this comment

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

@mchoma sorry, my explanation isn't really a good one; we indeed have resources -> *resources but then you see a field named as resources, and that can be confusing and make you think you have to use resources.resources to define it.

We could have resources -> *resources and then two fields like limits and requests or completely replace resources -> *resources by resources -> corev1.ResourceRequirements

Copy link
Contributor

Choose a reason for hiding this comment

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

or completely replace resources -> *resources by resources -> corev1.ResourceRequirements

Yes that was my understanding what could be done to be consistent with rest.

We could have resources -> *resources and then two fields like limits and requests

TBH that sounds to me it will be less consistent with rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto Pilot (V) Features for Phase V of the Operator capability levels Basic Install (I) Features for Phase I of the Operator capability levels enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix operator to support autoscaling with HPA
5 participants