Skip to content

Make Deployment reconciliation level-based.#1322

Merged
google-prow-robot merged 1 commit intoknative:masterfrom
mattmoor:moar-revisionist-revisioning
Jun 25, 2018
Merged

Make Deployment reconciliation level-based.#1322
google-prow-robot merged 1 commit intoknative:masterfrom
mattmoor:moar-revisionist-revisioning

Conversation

@mattmoor
Copy link
Copy Markdown
Member

@mattmoor mattmoor commented Jun 22, 2018

This starts to push the current switch over ServingState in the main Reconcile method into the methods that reconcile individual resources. As reconcileFoo handles the complete reconciliation of the Revision Foo resource, we will hoist it above the switch until the {create,delete}K8sResources methods are empty, at which point we will delete the switch.

This is WIP until the baseline change (#1321) goes in.

@google-prow-robot google-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 22, 2018
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 22, 2018

/retest

@mattmoor mattmoor force-pushed the moar-revisionist-revisioning branch from b1feee8 to 4ec9a0f Compare June 22, 2018 15:09
@google-prow-robot google-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 22, 2018
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 22, 2018

/test pull-knative-serving-unit-tests

@mattmoor mattmoor force-pushed the moar-revisionist-revisioning branch from 4ec9a0f to 8bac873 Compare June 22, 2018 21:11
@google-prow-robot google-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 22, 2018
@mattmoor mattmoor changed the title [WIP] Make Deployment reconciliation level-based. Make Deployment reconciliation level-based. Jun 22, 2018
@google-prow-robot google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2018
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 22, 2018

@josephburnett Note that this moves the experiment flag checks into the functions they were guarding.

@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 22, 2018

/test pull-knative-serving-integration-tests

@mattmoor mattmoor mentioned this pull request Jun 22, 2018
Merged
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 22, 2018

/hold

I suspect this is a real bug, so running conformance against a local deployment now.

@google-prow-robot google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2018
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 23, 2018

Yeah, there's definitely a bug right now:

  # Configuration status
  status:
    conditions:
    - lastTransitionTime: 2018-06-23T01:04:33Z
      status: "True"
      type: Ready
    latestCreatedRevisionName: service-example-00001
    latestReadyRevisionName: service-example-00001

  # Revision status
  status:
    conditions:
    - lastTransitionTime: 2018-06-23T01:05:16Z
      status: "True"
      type: ResourcesAvailable
    - lastTransitionTime: 2018-06-23T01:05:16Z
      status: "True"
      type: ContainerHealthy
    - lastTransitionTime: 2018-06-23T01:05:16Z
      status: "True"
      type: Ready

If you look at the timestamps, the Configuration sees the Revision become Ready: True at 04:33, but then it snaps back to Ready: False until a resync at 05:16.

@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 23, 2018

It's trying to reconcile away the defaults added to the Deployment. I'm wondering if we can call the Deployment defaulter in MakeServingDeployment.

@mattmoor mattmoor force-pushed the moar-revisionist-revisioning branch 3 times, most recently from 306d383 to dea2b13 Compare June 23, 2018 04:20
@mattmoor mattmoor mentioned this pull request Jun 23, 2018
Closed
@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 25, 2018

Don't let the test "failure" fool you, the error indicates it's because of an 80% coverage threshold, which is a pretty tight bound given our current standing. This should get us safely north of 80%.

@mattmoor
Make Deployment reconciliation level-based.
789ed6f 
This starts to push the current `switch` over `ServingState` in the main `Reconcile` method into the methods that reconcile individual resources.  As `reconcileFoo` handles the complete reconciliation of the `Revision` `Foo` resource, we will hoist it above the switch until the `{create,delete}K8sResources` methods are empty, at which point we will delete the switch.
@mattmoor mattmoor force-pushed the moar-revisionist-revisioning branch from dea2b13 to 789ed6f Compare June 25, 2018 15:43
@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Jun 25, 2018

/test pull-knative-serving-go-coverage

2 similar comments
@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Jun 25, 2018

/test pull-knative-serving-go-coverage

@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Jun 25, 2018

/test pull-knative-serving-go-coverage

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 25, 2018

/lgtm
/approve

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
@google-prow-robot
Copy link
Copy Markdown

google-prow-robot commented Jun 25, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas-google

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:

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

@mattmoor
Copy link
Copy Markdown
Member Author

mattmoor commented Jun 25, 2018

/hold cancel

Forgot to remove this after fixing things.

@google-prow-robot google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2018
@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Jun 25, 2018

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/controller/configuration/configuration.go 88.0% 87.1% -0.9
pkg/controller/service/service.go 84.6% 83.7% -0.9
pkg/controller/route/route.go 79.1% 78.9% -0.2
pkg/controller/revision/revision.go 77.4% 78.1% 0.7

*TestCoverage feature is being tested, do not rely on any info here yet

@google-prow-robot google-prow-robot merged commit acb6027 into knative:master Jun 25, 2018
google-prow-robot pushed a commit that referenced this pull request Jun 25, 2018
@mattmoor @google-prow-robot
This hoists the K8s Service reconciliation and makes it level-based. (#…
60cfc29 
…1328)

This is the next phase of our level-based migration of the Revision controller.  It includes the K8s Service for the user Deployment. This is patterned in much the same way as the Deployment refactor in #1322.  The most notable difference in this change is that we establish readiness from the Endpoints resource instead of the Service resource directly.

In addition, I spent a bit of time cleaning up some of the `revision_test.go` code to more automatically update the informers as resources are reconciled.  In particular, after `controller.Reconcile` we need to make sure all of the updates to the underlying objects are reflected in our informers, or a subsequent reconcile may act on stale state.  To facilitate this, I added a helper that updates the informers with the Revision/Deployment/Service to be used after a `controller.Reconcile`.
@dprotaso
Copy link
Copy Markdown
Member

dprotaso commented Jun 26, 2018 •
edited
Loading

Is there a defaulter for Deployment that we can use - similar to what @grantr's been working on here #1262?

Changes look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@grantr grantr

@dprotaso dprotaso

@vaikas vaikas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.

7 participants

@mattmoor @steuhs @vaikas @google-prow-robot @knative-metrics-robot @dprotaso @grantr