Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

fix iop status update issue.#741

Merged
istio-testing merged 5 commits intoistio:masterfrom
morvencao:br_fix_iop_status_update_issue
Jan 12, 2020
Merged

fix iop status update issue.#741
istio-testing merged 5 commits intoistio:masterfrom
morvencao:br_fix_iop_status_update_issue

Conversation

@morvencao
Copy link
Copy Markdown
Member

@morvencao morvencao commented Jan 9, 2020 •
edited
Loading

resolve: istio/istio#20021

also fix e2e-operator test.

@morvencao morvencao requested a review from a team as a code owner January 9, 2020 07:50
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 9, 2020
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 9, 2020
@morvencao morvencao force-pushed the br_fix_iop_status_update_issue branch from 9f7bd3d to df122b0 Compare January 9, 2020 09:34
dgn
dgn previously requested changes Jan 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgn dgn left a comment

Choose a reason for hiding this comment

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

I know this is not caused by this PR but rather the CRD, but I don't think this is the way to go. Status should always go into the Status part of the CR, never into the spec

Comment thread pkg/controller/istiocontrolplane/listeners.go Outdated
@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Jan 9, 2020

I know this is not caused by this PR but rather the CRD, but I don't think this is the way to go. Status should always go into the Status part of the CR, never into the spec

+1

@morvencao
Copy link
Copy Markdown
Member Author

morvencao commented Jan 10, 2020

This also fixes e2e_operatortest.

@morvencao
Copy link
Copy Markdown
Member Author

morvencao commented Jan 10, 2020

Raised a PR to move status fields out of IstioOperator spec: istio/api#1231 @ostromart PTAL

@morvencao morvencao mentioned this pull request Jan 10, 2020
Merged
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 11, 2020
@morvencao
Copy link
Copy Markdown
Member Author

morvencao commented Jan 11, 2020

Moved status out of spec:

# kubectl get iop -o yaml
apiVersion: v1
items:
- apiVersion: install.istio.io/v1alpha1
  kind: IstioOperator
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"install.istio.io/v1alpha1","kind":"IstioOperator","metadata":{"annotations":{},"name":"example-istiocontrolplane","namespace":"default"},"spec":{"profile":"demo"}}
    creationTimestamp: "2020-01-11T01:51:06Z"
    finalizers:
    - istio-finalizer.install.istio.io
    generation: 1
    name: example-istiocontrolplane
    namespace: default
    resourceVersion: "1540"
    selfLink: /apis/install.istio.io/v1alpha1/namespaces/default/istiooperators/example-istiocontrolplane
    uid: f52c3b52-53ea-43bd-8c87-bc84abfa4525
  spec:
    profile: demo
  status:
    component_status:
      Addon:
        status: 3
        status_string: HEALTHY
      Base:
        status: 3
        status_string: HEALTHY
      Citadel:
        status: 3
        status_string: HEALTHY
      EgressGateways:
        status: 3
        status_string: HEALTHY
      Galley:
        status: 3
        status_string: HEALTHY
      IngressGateways:
        status: 3
        status_string: HEALTHY
      Pilot:
        status: 3
        status_string: HEALTHY
      Policy:
        status: 3
        status_string: HEALTHY
      SidecarInjector:
        status: 3
        status_string: HEALTHY
      Telemetry:
        status: 3
        status_string: HEALTHY
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

@elfinhe elfinhe requested a review from ostromart January 12, 2020 05:23
elfinhe
elfinhe reviewed Jan 12, 2020
View reviewed changes
Comment thread pkg/helmreconciler/reconciler.go
wg.Wait()

out := &v1alpha1.InstallStatus{
//TODO: add overall status logic
Copy link
Copy Markdown
Member

@elfinhe elfinhe Jan 12, 2020

Choose a reason for hiding this comment

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

maybe we need an issue to track this.

elfinhe
elfinhe approved these changes Jan 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@elfinhe elfinhe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally LGTM. This unblocks e2e-operator test, I would let it merged first. @ostromart ptal if you have a second thought.

@elfinhe elfinhe requested a review from dgn January 12, 2020 05:27
@morvencao morvencao dismissed dgn’s stale review January 12, 2020 12:28

Addressed the comments.

@istio-testing istio-testing merged commit 4b27ee6 into istio:master Jan 12, 2020
@morvencao morvencao deleted the br_fix_iop_status_update_issue branch January 12, 2020 12:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ostromart ostromart Awaiting requested review from ostromart

@dgn dgn Awaiting requested review from dgn

2 more reviewers

@irisdingbj irisdingbj irisdingbj left review comments

@elfinhe elfinhe elfinhe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ostromart ostromart

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Operator reconciler always gets CR not found reconciling error

8 participants

@morvencao @howardjohn @elfinhe @dgn @irisdingbj @googlebot @istio-testing @ostromart