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

Rework how statuses are written during dag rebuild #1425

Closed
davecheney opened this issue Sep 2, 2019 · 1 comment · Fixed by #1745
Closed

Rework how statuses are written during dag rebuild #1425

davecheney opened this issue Sep 2, 2019 · 1 comment · Fixed by #1745
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@davecheney
Copy link
Contributor

Currently status writing is handled as part of the CacheHandler.OnUpdate operation. The dag.DAG contains a Statuses method which returns status objects collected during the dag build. This approach has several limitations

1a. Status writing causes the cache handler to have a reference to the Contour k8s Client so it can write status back to the API server. Ideally the CacheHandler would have no knowledge of k8s at this point--its role is to manage the internal/contour visitors and hold their output.
1b. Visitation occurs after DAG build so any problems encountered by the visitors should by unrelated to k8s objects as the DAG abstracts said objects.
2. Status update has always been a second class citizen. Because and continuing this fact is there are few tests of status because they occur outside the dag build cycle, and the ones that we do have are specific to IngressRoute. There are no e2e tests of status.
3. We only report status for IngressRoute objects, not Ingress, not HTTPLoadBalancer, etc.
4. Status updates are tied to dag regeneration and this has a long history of producing duplicates, not to mention multiple contours trying to update the same object.

I propose introducing a layer of abstraction conceptual equivalent to a logger. To set the context, with Logrus we pass down into various things a FieldLogger, which is an object which collects bits of context along the way. At the bottom of the call chain, when something wants to log it doesn't need to have all the context of the various pieces that were added to the logger's context on the way down, they're already present in the FieldLogger.

I'm proposing something similar for status, a status logger or writer which collects context as it is used. The primary context is obviously the k8s object that is being worked on at the moment, but as we dig deeper into the object it might be a route, a service connected to a route, etc. There are also aspects of the builder pattern as we want to collect multiple units of status per object then emit them once

The overall API might look something like this

type StatusWriter interface {
        WithObject(obj interface{}) ObjectStatusWriter
}

type ObjectStatusWriter {
      
        // various logging methods
  
        // Commit writes the colletive status back to the API server
        Commit() error
}

This would let us decouple anything wanting to write status from the actual status writing operation itself. WithObject(obj).Commit() would be a noop if there was no status to write. It would also let us test status writing directly to the fake k8s client rather than a side effect of dag building.

@davecheney davecheney added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 2, 2019
@davecheney davecheney added this to the 1.0.0-beta.1 milestone Sep 2, 2019
@davecheney davecheney self-assigned this Sep 2, 2019
davecheney added a commit to davecheney/contour that referenced this issue Sep 2, 2019
Updates projectcontour#1425

Move the k8s.IngressRouteStatus object from the CacheHandler to the
EventHandler. In doing so the CacheHandler is only responsible for
updating itself via its visitors when presented with a new DAG.

Signed-off-by: Dave Cheney <dave@cheney.net>
davecheney added a commit that referenced this issue Sep 2, 2019
Updates #1425

Move the k8s.IngressRouteStatus object from the CacheHandler to the
EventHandler. In doing so the CacheHandler is only responsible for
updating itself via its visitors when presented with a new DAG.

Signed-off-by: Dave Cheney <dave@cheney.net>
@davecheney davecheney changed the title Rework how statuses are writing during dag rebuild Rework how statuses are written during dag rebuild Sep 2, 2019
@stevesloka
Copy link
Member

@davecheney is this all fixed up with #1475?

davecheney added a commit to davecheney/contour that referenced this issue Sep 25, 2019
Updates projectcontour#1425
Updates projectcontour#1449

They're not different enough to carry the duplication.

Signed-off-by: Dave Cheney <dave@cheney.net>
davecheney added a commit that referenced this issue Sep 25, 2019
Updates #1425
Updates #1449

They're not different enough to carry the duplication.

Signed-off-by: Dave Cheney <dave@cheney.net>
@davecheney davecheney modified the milestones: 1.0.0-beta.1, 1.0.0-rc.1 Sep 26, 2019
@davecheney davecheney modified the milestones: 1.0.0-rc.1, 1.0.0-rc.2 Oct 7, 2019
davecheney added a commit to davecheney/contour that referenced this issue Oct 21, 2019
Fixes projectcontour#1425
Fixes projectcontour#1385
Updates projectcontour#499

This PR threads the leader elected signal throught to
contour.EventHandler allowing it to skip writing status back to the API
unless it is currently the leader.

This should fixes projectcontour#1425 by removing the condition where several Contours
would fight to update status. This updates projectcontour#499 by continuing to reduce
the number of updates that Contour generates, thereby processes.

This PR does create a condition where during startup no Contour may be
the leader and the xDS tables reach steady state before anyone is
elected. This would mean the status of an object would be stale until
the next update from the API server after leadership was established.
To address this a mechanism to force a rebuild of the dag is added to
the EventHandler and wired to election success.

Signed-off-by: Dave Cheney <dave@cheney.net>
davecheney added a commit that referenced this issue Oct 21, 2019
Fixes #1425
Fixes #1385
Updates #499

This PR threads the leader elected signal throught to
contour.EventHandler allowing it to skip writing status back to the API
unless it is currently the leader.

This should fixes #1425 by removing the condition where several Contours
would fight to update status. This updates #499 by continuing to reduce
the number of updates that Contour generates, thereby processes.

This PR does create a condition where during startup no Contour may be
the leader and the xDS tables reach steady state before anyone is
elected. This would mean the status of an object would be stale until
the next update from the API server after leadership was established.
To address this a mechanism to force a rebuild of the dag is added to
the EventHandler and wired to election success.

Signed-off-by: Dave Cheney <dave@cheney.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants