Skip to content

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

Merged
sleshchenko merged 3 commits into
devfile:mainfrom
metlos:ignore-service-clusterips
Jul 28, 2021
Merged

fix: Ignore serviceSpec.ClusterIPs when comparing services to prevent endless routing reconciliation loop#504
sleshchenko merged 3 commits into
devfile:mainfrom
metlos:ignore-service-clusterips

Conversation

@metlos
Copy link
Copy Markdown
Contributor

@metlos metlos commented Jul 23, 2021 •
edited by sleshchenko
Loading

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
@metlos
fix: Ignore serviceSpec.ClusterIPs when comparing services to prevent…
36f25bc 
… 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
Copy Markdown
Member

sleshchenko commented Jul 23, 2021

/test v7-devworkspaces-operator-e2e

@openshift-ci
Copy link
Copy Markdown

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

Details 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
Copy Markdown
Collaborator

amisevsk commented Jul 23, 2021

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

1 similar comment
@metlos
Copy link
Copy Markdown
Contributor Author

metlos commented Jul 23, 2021

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

@metlos metlos mentioned this pull request Jul 23, 2021
Merged
11 tasks
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jul 23, 2021 •
edited
Loading

@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.

Details

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
Copy Markdown
Member

sleshchenko commented Jul 26, 2021

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
Copy Markdown
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).

@sleshchenko sleshchenko mentioned this pull request Jul 26, 2021
Closed
@openshift-ci openshift-ci Bot removed the lgtm label Jul 26, 2021
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jul 26, 2021

New changes are detected. LGTM label has been removed.

@JPinkney JPinkney mentioned this pull request Jul 26, 2021
Closed
8 tasks
@metlos metlos mentioned this pull request Jul 27, 2021
Closed
@metlos
Copy link
Copy Markdown
Contributor Author

metlos commented Jul 27, 2021

/test

@openshift-ci
Copy link
Copy Markdown

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
Details

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
Copy Markdown
Contributor Author

metlos commented Jul 27, 2021

/test v7-devworkspaces-operator-e2e

@sleshchenko
Copy link
Copy Markdown
Member

sleshchenko commented Jul 27, 2021

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

@sleshchenko
Copy link
Copy Markdown
Member

sleshchenko commented Jul 28, 2021

/test v7-devworkspace-happy-path

@sleshchenko
Copy link
Copy Markdown
Member

sleshchenko commented Jul 28, 2021

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
Copy Markdown
Member

sleshchenko commented Jul 28, 2021

/test v7-devworkspace-happy-path

@sleshchenko
Copy link
Copy Markdown
Member

sleshchenko commented Jul 28, 2021

Happy path passed https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/devfile_devworkspace-operator/504/pull-ci-devfile-devworkspace-operator-main-v7-devworkspace-happy-path/1420344997298835456/artifacts/devworkspace-happy-path/devworkspace-happy-path/build-log.txt, the cluster is being deprovisioned

@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

Reviewers

@JPinkney JPinkney JPinkney approved these changes

@amisevsk amisevsk amisevsk approved these changes

+1 more reviewer

@sleshchenko sleshchenko sleshchenko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sleshchenko sleshchenko

Labels

approved

Projects

None yet

Milestone

v0.8.0

Development

Successfully merging this pull request may close these issues.

4 participants

@metlos @sleshchenko @amisevsk @JPinkney