Skip to content

fix: Ignore serviceSpec.ClusterIPs when comparing services to prevent endless routing reconciliation loop #504

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

Merged
merged 3 commits into from
Jul 28, 2021

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Jul 23, 2021

What does this PR do?

The field clusterIPs was introduced in Kubernetes 1.20. We're not ignoring it when reconciling the services and because it is automatically set by Kubernetes, it causes a permanent difference between the intended spec and the actual spec preventing the controller from finishing the reconciliation of a devworkspace routing.

What issues does this PR fix or reference?

N/A

Is it tested? How?

Only manually.

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
    • v7-devworkspaces-operator-e2e: DevWorkspace e2e test
    • v7-devworkspace-happy-path: DevWorkspace e2e test

@openshift-ci openshift-ci bot requested a review from JPinkney July 23, 2021 11:46
@metlos metlos changed the title Ignore serviceSpec.ClusterIPs when comparing services to prevent endless routing reconciliation loop fix: Ignore serviceSpec.ClusterIPs when comparing services to prevent endless routing reconciliation loop Jul 23, 2021
… endless

routing r<!-- Please provide instructions here how reviewer can test your changes if applicable -->econciliation loop.

The field was introduced in Kubernetes 1.20.
@metlos metlos force-pushed the ignore-service-clusterips branch from f442226 to 36f25bc Compare July 23, 2021 11:47
@sleshchenko
Copy link
Member

/test v7-devworkspaces-operator-e2e

@openshift-ci
Copy link

openshift-ci bot commented Jul 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, metlos, sleshchenko

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:
  • OWNERS [JPinkney,amisevsk,sleshchenko]

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

@amisevsk
Copy link
Collaborator

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

1 similar comment
@metlos
Copy link
Contributor Author

metlos commented Jul 23, 2021

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@openshift-ci
Copy link

openshift-ci bot commented Jul 23, 2021

@metlos: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/v7-devworkspace-happy-path 36f25bc link /test v7-devworkspace-happy-path

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@sleshchenko
Copy link
Member

from e2e test, devworkspace-controller:

panic: ClusterIPs: does not exist

goroutine 1 [running]:
github.com/google/go-cmp/cmp/cmpopts.newStructFilter(0x17f82e0, 0xc000448000, 0xc000430e70, 0x3, 0x3, > 0x1a612a0, 0xc000131710, 0x40ae1f, 0x17f82e0)

@metlos
Copy link
Contributor Author

metlos commented Jul 26, 2021

Hmm... so this boils down to the difference in k8s.io/api version in che-operator (0.20.6) vs devworkspace-operator (0.18.8).

@openshift-ci openshift-ci bot removed the lgtm label Jul 26, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 26, 2021

New changes are detected. LGTM label has been removed.

@metlos
Copy link
Contributor Author

metlos commented Jul 27, 2021

/test

@openshift-ci
Copy link

openshift-ci bot commented Jul 27, 2021

@metlos: The /test command needs one or more targets.
The following commands are available to trigger jobs:

  • /test v7-devworkspace-happy-path
  • /test v7-devworkspaces-operator-e2e
  • /test v7-images

Use /test all to run the following jobs:

  • pull-ci-devfile-devworkspace-operator-main-v7-images

In response to this:

/test

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.

@metlos
Copy link
Contributor Author

metlos commented Jul 27, 2021

/test v7-devworkspaces-operator-e2e

@sleshchenko
Copy link
Member

Tests passed. I'll take care of it and sync with PR on devfile/api side

@sleshchenko
Copy link
Member

/test v7-devworkspace-happy-path

@sleshchenko
Copy link
Member

I have troubles with updated DWO to devfile/api#565.

Tested the current state and everything seems to work:

  • DevWorkspace withing Che;
  • WebTerminal including owner only access;
    So, triggered Che happy path and going to merge it after it's passed.
    Upgrading to devfile with newer k8s/api and controller-runtime is going to be handled in a separate issue Update K8s API to newest #516

@sleshchenko sleshchenko reopened this Jul 28, 2021
@sleshchenko
Copy link
Member

/test v7-devworkspace-happy-path

@sleshchenko sleshchenko merged commit 108b1f1 into devfile:main Jul 28, 2021
@sleshchenko sleshchenko added this to the v0.8.0 milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants