fixing listing nodes #1739

jesskranz · 2022-03-08T11:37:56Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

aks_api = client.CoreV1Api()
response = aks_api.list_node()

While listing the nodes i'm getting errors, because types were not in the allow list. Not sure what these types are, but adding these allowed me to list the nodes in my cluster.

Which issue(s) this PR fixes:

Fixes #1735

Special notes for your reviewer:

This is my first contribution to an open source project, please help me go through the steps

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2022-03-08T11:37:58Z

❌ login: jesskranz / name: Jess (39ffe65). This user is authorized, but they must confirm their affiliation with their company. Start the authorization process by clicking here, click "Corporate",select the appropriate company from the list, then confirm your affiliation on the page that appears. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2022-03-08T11:38:03Z

Welcome @jesskranz!

It looks like this is your first PR to kubernetes-client/python 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-03-08T11:38:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jesskranz
To complete the pull request process, please assign roycaihw after the PR has been reviewed.
You can assign the PR to them by writing /assign @roycaihw in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

yliaog · 2022-03-08T17:00:50Z

the file is auto generated, please apply any fix upstream

roycaihw · 2022-03-08T17:56:10Z

kubernetes/client/models/v1_node_condition.py

@@ -214,12 +214,13 @@ def type(self, type):
        """
        if self.local_vars_configuration.client_side_validation and type is None:  # noqa: E501
            raise ValueError("Invalid value for `type`, must not be `None`")  # noqa: E501
-        allowed_values = ["DiskPressure", "MemoryPressure", "NetworkUnavailable", "PIDPressure", "Ready"]  # noqa: E501
+        allowed_values = ["DiskPressure", "MemoryPressure", "NetworkUnavailable", "PIDPressure", "Ready", "TerminateScheduled", "PreemptScheduled", "FreezeScheduled", "RebootScheduled", "FrequentContainerdRestart", "RedeployScheduled", "ReadonlyFilesystem", "FrequentKubeletRestart", "ContainerRuntimeProblem", "FilesystemCorruptionProblem", "FrequentDockerRestart", "KubeletProblem", "KernelDeadlock","FrequentUnregisterNetDevice"]  # noqa: E501


I think this needs to be fixed in the upstream kubernetes. This client is generated from Kubernetes OpenAPI spec, and according to the spec:

"type": { "description": "Type of node condition.\n\nPossible enum values:\n - `\"DiskPressure\"` means the kubelet is under pressure due to insufficient available disk.\n - `\"MemoryPressure\"` means the kubelet is under pressure due to insufficient available memory.\n - `\"NetworkUnavailable\"` means that network for the node is not correctly configured.\n - `\"PIDPressure\"` means the kubelet is under pressure due to insufficient available PID.\n - `\"Ready\"` means kubelet is healthy and ready to accept pods.", "enum": [ "DiskPressure", "MemoryPressure", "NetworkUnavailable", "PIDPressure", "Ready" ], "type": "string" }

@roycaihw you reckon we should make a PR over there? any guidelines on where should we add this ?

Yes, I'm afraid we should remove this line.

kubernetes/kubernetes#105057 made the openapi-generated clients do more strict validation for this enum. However, according to #1735 it seems common for cloud providers (e.g. GKE, AKS) to expend the possible values for this enum-- which won't pass the client-side validation.

In the meantime, I think we can accept a hot patch to remove this client-side validation. It seems pretty bad that the client will randomly fail listing nodes when talking to a cloud provider.

I think we should treat this field to be not an enum. I will send a PR shortly.

yliaog · 2022-03-10T02:19:37Z

@jiahuif @apelisse @jpbetz

kubernetes/kubernetes#105057 broke python client

roycaihw · 2022-03-10T17:38:44Z

#1739 (comment): +1. It may affect other openapi-generated clients / other APIs as well

jiahuif · 2022-03-10T18:08:57Z

This is expected. KEP-2887 change all fields that are actually enums to be enum in the OpenAPI spec.

jiahuif · 2022-03-10T18:10:59Z

kubernetes/client/models/v1_node_condition.py

        if self.local_vars_configuration.client_side_validation and type not in allowed_values:  # noqa: E501
            raise ValueError(
                "Invalid value for `type` ({0}), must be one of {1}"  # noqa: E501
                .format(type, allowed_values)
            )
+


nit: stray edit.

jiahuif

Can we postpone this PR a bit? I will send a PR to k/k to mark this field to be non-enum.

yliaog · 2022-03-10T18:13:22Z

#1735
#1733
fyi it is more than just that single field.

jiahuif · 2022-03-10T18:45:35Z

xref: kubernetes/kubernetes#108639

roycaihw · 2022-03-10T19:16:25Z

Can we postpone this PR a bit? I will send a PR to k/k to mark this field to be non-enum.

@jiahuif Would be great if k/k can also do a cherrypick. Otherwise this client (and other clients that are affected) has to either wait for a new k/k minor release, or do its own hot patch.

tanguyfalconnet · 2022-03-15T16:09:08Z

+1 it shouldn't be a static enum.
I am using node-problem-detector to create my own conditions (to check dns state for example).

jiahuif · 2022-03-16T22:43:23Z

backport to 1.23 pending approval as kubernetes/kubernetes#108740

roycaihw · 2022-03-28T16:37:51Z

The upstream openapi spec is fixed (in master). Once the cherrypick https://github.com/kubernetes/kubernetes/pull/108740/files is merged we can cut a new 23 client to fix this issue.
/close

k8s-ci-robot · 2022-03-28T16:37:53Z

@roycaihw: Closed this PR.

In response to this:

The upstream openapi spec is fixed (in master). Once the cherrypick https://github.com/kubernetes/kubernetes/pull/108740/files is merged we can cut a new 23 client to fix this issue.
/close

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/test-infra repository.

fixing listing nodes

39ffe65

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Mar 8, 2022

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 8, 2022

k8s-ci-robot requested review from roycaihw and yliaog March 8, 2022 11:38

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 8, 2022

jesskranz mentioned this pull request Mar 8, 2022

client.CoreV1Api().list_node() does not work #1735

Closed

roycaihw reviewed Mar 8, 2022

View reviewed changes

jiahuif reviewed Mar 10, 2022

View reviewed changes

jiahuif mentioned this pull request Mar 10, 2022

remove enum markers on types without validation kubernetes/kubernetes#108639

Merged

k8s-ci-robot closed this Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing listing nodes #1739

fixing listing nodes #1739

jesskranz commented Mar 8, 2022

linux-foundation-easycla bot commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

yliaog commented Mar 8, 2022

roycaihw Mar 8, 2022

dav009 Mar 9, 2022

roycaihw Mar 10, 2022

roycaihw Mar 10, 2022

jiahuif Mar 10, 2022

yliaog commented Mar 10, 2022

roycaihw commented Mar 10, 2022 •

edited

jiahuif commented Mar 10, 2022

jiahuif Mar 10, 2022

jiahuif left a comment

yliaog commented Mar 10, 2022

jiahuif commented Mar 10, 2022

roycaihw commented Mar 10, 2022

tanguyfalconnet commented Mar 15, 2022

jiahuif commented Mar 16, 2022

roycaihw commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

fixing listing nodes #1739

fixing listing nodes #1739

Conversation

jesskranz commented Mar 8, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

yliaog commented Mar 8, 2022

roycaihw Mar 8, 2022

Choose a reason for hiding this comment

dav009 Mar 9, 2022

Choose a reason for hiding this comment

roycaihw Mar 10, 2022

Choose a reason for hiding this comment

roycaihw Mar 10, 2022

Choose a reason for hiding this comment

jiahuif Mar 10, 2022

Choose a reason for hiding this comment

yliaog commented Mar 10, 2022

roycaihw commented Mar 10, 2022 • edited

jiahuif commented Mar 10, 2022

jiahuif Mar 10, 2022

Choose a reason for hiding this comment

jiahuif left a comment

Choose a reason for hiding this comment

yliaog commented Mar 10, 2022

jiahuif commented Mar 10, 2022

roycaihw commented Mar 10, 2022

tanguyfalconnet commented Mar 15, 2022

jiahuif commented Mar 16, 2022

roycaihw commented Mar 28, 2022

k8s-ci-robot commented Mar 28, 2022

roycaihw commented Mar 10, 2022 •

edited