Mpuncel/rc audit logs #877
Conversation
| @@ -396,7 +401,8 @@ func (rc *replicationController) ensureConsistency(current types.PodLocations) e | |||
| } | |||
|
|
|||
| rc.logger.WithField("node", pod.Node).WithField("intentManifestSHA", intentSHA).Info("Found inconsistency in scheduled manifest") | |||
| if err := rc.schedule(ctx, pod.Node); err != nil { | |||
|
|
|||
| if err := rc.scheduleNoAudit(ctx, pod.Node); err != nil { | |||
There was a problem hiding this comment.
potentially we could just do audit logging in ensureConsistency() as well. it's going to produce an extra audit log record unnecessarily which is a downside but the upside would mean we don't need this scheduleNoAudit() function and we also don't need the Context() getter on the auditing transaction, and it would prevent someone from doing transaction.Commit(txn.Context() in the wrong place (and thus not creating an audit log record)
There was a problem hiding this comment.
actually we need that Context() getter unless we're going to move node scheduling logic into the auditing transaction as well. i think things are fine the way they are
mpuncel
force-pushed
the
mpuncel/rc-audit-logs
branch
3 times, most recently
from
3705d4d
to
7c75acf
Compare
pkg/pc/fields/fields.go
Outdated
| PodIDLabel = rcstore.PodIDLabel // TODO: put this in a different place now that multiple packages use it | ||
| AvailabilityZoneLabel = types.AvailabilityZoneLabel | ||
| ClusterNameLabel = types.ClusterNameLabel | ||
| PodIDLabel = types.PodIDLabel // TODO: put this in a different place now that multiple packages use it |
There was a problem hiding this comment.
remove the TODO comment because it has been satisfied?
|
|
||
| type auditingTransaction struct { | ||
| ctx context.Context | ||
| nodes map[types.NodeName]struct{} |
There was a problem hiding this comment.
why is this a map of types.NodeName to struct{}? I've been using map[types.NodeName]bool to create sets for fast lookup. is this analogous?
There was a problem hiding this comment.
I could have done bool, but struct{} occupies 0 bytes of memory so it's slightly more "efficient" even though it doesn't really matter here.
mpuncel
force-pushed
the
mpuncel/rc-audit-logs
branch
from
7c75acf
to
8cd9319
Compare
pkg/rc/replication_controller.go
Outdated
| @@ -226,19 +229,19 @@ func (rc *replicationController) addPods(current types.PodLocations) error { | |||
|
|
|||
| rc.logger.NoFields().Infof("Need to schedule %d nodes out of %s", toSchedule, possible) | |||
|
|
|||
| ctx, cancelFunc := transaction.New(context.Background()) | |||
| txn, cancelFunc := rc.newAuditingTransaction(context.Background(), currentNodes) | |||
| for i := 0; i < toSchedule; i++ { | |||
| // create a new context for every 5 nodes. This is done to make sure | |||
There was a problem hiding this comment.
Unrelated to the PR here, but would you mind adding a link to this issue in this comment? hashicorp/consul#2921
We'll be able to simplify this code soon.
|
What kind of metrics should we watch when this rolls out? I think it could cause a write spike, especially for pods like p2-heartbeat |
|
I don't think it will show up in our metrics, we're not increasing the number of operations performed by the RC farm, we're just adding an extra write in each operation. Bandwidth will increase but it will likely be invisible compared to other sources of bandwidth fluctuation |
mpuncel
force-pushed
the
mpuncel/rc-audit-logs
branch
from
f5f3c6c
to
93d987d
Compare
This allows expanding the labels.ApplicatorWithoutWatches interface which makes it easier to use in various binaries that switch on using HTTP applicators vs direct consul access.
An rc retargeting event represents an RC changing the set of nodes it targets, e.g. due to a replica count change, selector change, or a change to the labels of a node. This commit creates the json schema for the details for an event as well as a constructor for easily creating the json.RawMessage for use in an audit log record.
RCs have a "pod_labels" field which represents a set of labels that should be applied to every pod that is scheduled by that RC. Previously there has been no requirement that that set of labels includes the availability_zone and cluster_name to identify the pod cluster that the RC and its pods are members of. pod_id can be inferred from the pod manifest and is already automatically set. This commit enforces that the set of labels have availability zone and cluster name in anticipation of having RCs create audit log records every time the set of nodes they target changes. This is because audit log records are linked to pod clusters, and as a result RCs must be as well.
An audit log record will be created whenever nodes are scheduled or unscheduled by an RC. This will allow constructing a history of which nodes were managed by an RC or pod cluster over time.
mpuncel
force-pushed
the
mpuncel/rc-audit-logs
branch
from
93d987d
to
087ea9e
Compare
No description provided.