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

Diff on Endpoint confused by IP sorting #1816

Closed
warmfusion opened this issue Jun 25, 2019 · 10 comments · Fixed by argoproj/gitops-engine#160 or #4552
Closed

Diff on Endpoint confused by IP sorting #1816

warmfusion opened this issue Jun 25, 2019 · 10 comments · Fixed by argoproj/gitops-engine#160 or #4552
Assignees
Labels
bug Something isn't working component:core Syncing, diffing, cluster state cache good first issue Good for newcomers help wanted Extra attention is needed workaround There's a workaround, might not be great, but exists
Milestone

Comments

@warmfusion
Copy link
Contributor

Describe the bug
When using an Endpoint object with a set of IP addresses explicitally declared, when comparing the state against the intention the diff appears to sort the IP addresses Alphanumerically whilst Kubernetes sorts by octet resulting in a mistaken diff output.

To Reproduce
Provide a configuration as shown;

apiVersion: v1
kind: Endpoints
metadata:
  annotations:
    description: A workaround to support a set of backend IPs for solr
    linkerd.io/inject: disabled
  name: solrcloud
  namespace: engineering-fcsis-vanilla-quantum
subsets:
- addresses:
  - ip: 172.20.10.97
  - ip: 172.20.10.98
  - ip: 172.20.10.99
  - ip: 172.20.10.100
  - ip: 172.20.10.101
  ports:
  - name: solr-http
    port: 8080

Expected behavior

State in kubernetes is applied and no diff errors are shown.

** Actual Behaviour **

The diff triggers an out-of-sync warning as the IP list gets changed by some part of the system.

Screenshots

Screen Shot 2019-06-25 at 14 09 42

Version

argocd: v1.0.1+5fe1447.dirty
  BuildDate: 2019-05-28T17:28:05Z
  GitCommit: 5fe1447b722716649143c63f9fc054886d5b111c
  GitTreeState: dirty
  GoVersion: go1.11.4
  Compiler: gc
  Platform: darwin/amd64
argocd-server: v1.0.1+5fe1447.dirty
  BuildDate: 2019-05-28T17:27:38Z
  GitCommit: 5fe1447b722716649143c63f9fc054886d5b111c
  GitTreeState: dirty
  GoVersion: go1.11.4
  Compiler: gc
  Platform: linux/amd64
  Ksonnet Version: 0.13.1
@warmfusion warmfusion added the bug Something isn't working label Jun 25, 2019
@alexec
Copy link
Contributor

alexec commented Jun 25, 2019

Interesting. Can you work around by making sure the Git repo has the ASCII ordered list, rather than the (and admittedly nicer) dictionary order.

@jessesuen
Copy link
Member

This is the same problem that HPA has. The only real workaround is to order the list in the same way that Kubernetes orders this list.

It's unlikely that Argo CD can do anything about this because we have no way to understand if list order is significant or not for all types of objects.

@alexec alexec added the workaround There's a workaround, might not be great, but exists label Jun 25, 2019
@warmfusion
Copy link
Contributor Author

The workaround works as a solution, albeit one that needs a little twiddling if changes are made.

Could there perhaps be a whitelist of known sortable elements for both HPA and endpoints for instance?

@stale
Copy link

stale bot commented Sep 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Sep 16, 2019
@abhishekjiitr
Copy link
Contributor

+1, really annoying for endpoint resources with lots of addresses

@abhishekjiitr
Copy link
Contributor

Interesting. Can you work around by making sure the Git repo has the ASCII ordered list, rather than the (and admittedly nicer) dictionary order.

This doesn't seem to work for ascii ordered list. Kubernetes seems to sort the list like this, which is not ascii ordered:

apiVersion: v1
kind: Endpoints
metadata:
  name: qa
  namespace: monitoring
subsets:
  - addresses:
      - ip: 10.2.4.5
    ports:
      - name: bindexporter
        port: 9119
        protocol: TCP
      - name: nodeexporter
        port: 9100
        protocol: TCP
      - name: postgresexporter
        port: 9187
        protocol: TCP
      - name: apacheexporter
        port: 9117
        protocol: TCP
      - name: jmxexporter
        port: 7090
        protocol: TCP

@alexmt alexmt removed the wontfix This will not be worked on label Jan 27, 2020
@jannfis jannfis added the component:core Syncing, diffing, cluster state cache label May 14, 2020
@alexmt alexmt added this to the v1.7 milestone May 20, 2020
@darshanime
Copy link
Member

Currently, the IPs in endpoints are sorted lexicographically
https://github.com/kubernetes/kubernetes/blob/2da917d3701904939683ac545c7869c1e89b7840/pkg/api/endpoints/util.go#L191-L200

In argo-cd, how about sorting the manifest values using this function before comparing them for diffing?


To solve this more generally, is it a good idea to introduce a "normalizing" step before diffing which takes care of such intricacies based on a global mapping of GVK-field <> sorting order.

@alexmt alexmt modified the milestones: v1.7 , v1.8 Aug 25, 2020
@jessesuen jessesuen added help wanted Extra attention is needed good first issue Good for newcomers labels Sep 16, 2020
@jessesuen
Copy link
Member

I think a special case for Endpoints to lexographically sort the endpoints before diffing would be ok.

@wtam2018
Copy link
Contributor

wtam2018 commented Sep 17, 2020

/assign @wtam2018

@wtam2018
Copy link
Contributor

argoproj/gitops-engine#160 has been submitted to gitops-engine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:core Syncing, diffing, cluster state cache good first issue Good for newcomers help wanted Extra attention is needed workaround There's a workaround, might not be great, but exists
Projects
None yet
8 participants