Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Let fluxd answer queries without a git repo supplied #962

Merged
merged 4 commits into from
Mar 19, 2018
Merged

Let fluxd answer queries without a git repo supplied #962

merged 4 commits into from
Mar 19, 2018

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Feb 26, 2018 •
edited
Loading

Instead of waiting for the git repo to be ready before responding to API calls (aside from status), just start the repo syncing when starting up, and respond according to its current state.

The API (ListServices) will now mark controllers that aren't in the git repo -- or all controllers if there's no git repo -- as being read-only, supplying a reason in the field ReadOnly. An empty field indicates that the controller is (probably) writable. Old daemons won't populate this field, but since they don't answer ListServices until the repo is ready anyway, the assumption that the controller is writable will still usually be correct (and not disastrous when it's wrong).

An example API response:

[
  {
    "ID": "hello:cronjob/hellocron",
    "Containers": [
      {
        "Name": "weekly-hello-cron",
        "Current": {
          "ID": "alpine:3.4",
          "Digest": "",
          "ImageID": ""
        },
        "Available": null
      }
    ],
    "ReadOnly": "NotInRepo",
    "Status": "ready",
    "Automated": false,
    "Locked": false,
    "Ignore": false,
    "Policies": {}
  },
  {
    "ID": "hello:deployment/helloworld",
    "Containers": [
      {
        "Name": "helloworld",
        "Current": {
          "ID": "quay.io/weaveworks/helloworld:master-07a1b6b",
          "Digest": "",
          "ImageID": ""
        },
        "Available": null
      },
      {
        "Name": "sidecar",
        "Current": {
          "ID": "quay.io/weaveworks/sidecar:master-a000002",
          "Digest": "",
          "ImageID": ""
        },
        "Available": null
      }
    ],
    "Status": "ready",
    "Automated": false,
    "Locked": false,
    "Ignore": false,
    "Policies": {
      "tag.helloworld": "glob:master-*"
    }
  },
  {
    "ID": "hello:deployment/localhello",
    "Containers": [
      {
        "Name": "helloworld",
        "Current": {
          "ID": "kube-registry.kube-system.svc.cluster.local:5000/mikeb/hello",
          "Digest": "",
          "ImageID": ""
        },
        "Available": null
      },
      {
        "Name": "sidecar",
        "Current": {
          "ID": "quay.io/weaveworks/sidecar:master-a000002",
          "Digest": "",
          "ImageID": ""
        },
        "Available": null
      }
    ],
    "ReadOnly": "NotInRepo",
    "Status": "ready",
    "Automated": false,
    "Locked": false,
    "Ignore": false,
    "Policies": {}
  }
]

@squaremo squaremo changed the title [WIP] Let fluxd work (in some respects) without a git repo supplied Let fluxd answer queries without a git repo supplied Mar 2, 2018
@squaremo squaremo requested a review from samb1729 March 8, 2018 11:49
samb1729
samb1729 reviewed Mar 11, 2018
View reviewed changes
Copy link
Contributor

@samb1729 samb1729 left a comment

Choose a reason for hiding this comment

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

Few nitpicks, bit of code praise, some questions.

cmd/fluxd/main.go Outdated
"email", *gitEmail,
"sync-tag", *gitSyncTag,
"notes-ref", *gitNotesRef,
"set-author", *gitSetAuthor)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

cmd/fluxd/main.go
EventWriter: eventWriter,
Logger: log.With(logger, "component", "daemon"), LoopVars: &daemon.LoopVars{
Logger: log.With(logger, "component", "daemon"),
LoopVars: &daemon.LoopVars{

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

cluster/cluster.go Outdated
@@ -29,6 +29,10 @@ type Cluster interface {
type Controller struct {
ID flux.ResourceID
Status string // A status summary for display
// Is the controller considered read-only because it's under the
// control of the platform. In the case of Kibernetes, we simply

This comment was marked as abuse.

Sign in to view

cluster/cluster.go Outdated
// Is the controller considered read-only because it's under the
// control of the platform. In the case of Kibernetes, we simply
// omit these controllers; but this may not always be the case.
IsSystem bool

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

api/v6/api.go Outdated
type ControllerStatus struct {
ID flux.ResourceID
Containers []Container
ReadOnly ReadOnlyReason `json:",omitempty"`

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

daemon/daemon.go
err = d.WithClone(ctx, func(checkout *git.Checkout) error {
var err error
services, err = d.Manifests.ServicesWithPolicies(checkout.ManifestDir())
return err
})
if err != nil {
switch {

This comment was marked as abuse.

Sign in to view

daemon/daemon.go Outdated
return nil, errors.Wrap(err, "getting service policies")
}

var res []v6.ControllerStatus
for _, service := range clusterServices {
policies := services[service.ID]
var readonly v6.ReadOnlyReason

This comment was marked as abuse.

Sign in to view

git/repo.go
@@ -21,7 +21,7 @@ const (
var (
ErrNoChanges = errors.New("no changes made in repo")
ErrNotReady = errors.New("git repo not ready")
ErrNoConfig = errors.New("git repo has not valid config")
ErrNoConfig = errors.New("git repo does not have valid config")

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@samb1729
Copy link
Contributor

Should we wait until we're ready to do a 1.3 release before we merge this? Or, conversely, should we more quickly make sure we're ready to do a 1.3 so that we can merge this?

squaremo added 3 commits March 12, 2018 12:58
@squaremo
Don't wait for git mirror to be ready
6d391f0 
Instead of waiting for the git repo to be cloned and ready, just start
the mirror going and be prepared for some operations to fail.

This is the first step towards operating without a git repo; the
second is to change (some) API responses depending on whether a git
repo is present, rather than failing when it's not.
@squaremo
Adapt minimally to the possible absence of a repo
eac880a 
Of the API methods, ListControllers is the only one for which a git
repo is optional; make it return a response with less information,
rather than an error, if the repo is not ready.

Some other methods either don't try to look at the repo (ListImages), in
which case no change is needed.

The remainder refer to the repo (usually because they want to write to
it), so must return an error. That will happen as things stand, since
attempting to clone the repo results in an error.
@squaremo
Mark read-only controllers as read-only
8f8dc0c 
Some controllers will be effectively read-only, because

 - they are under control of the system (e.g., addons)
 - they don't appear in the git repo, so we can't write changes
 - the git repo isn't ready, or hasn't been supplied

So that we can treat these specially, mark them as read-only in the
ListServices API result.

On backwards compatibility: I have added this into the v6 API, because
it's a hint to the API client; the zero value indicates it is
_probably_ OK to attempt whatever you want to attempt. Since prior
daemon code wouldn't proceed if the git repo wasn't ready, that is
more often than not the case.
@squaremo
Copy link
Member Author

Should we wait until we're ready to do a 1.3 release before we merge this?

I reckon we can do a release in the meantime.

@squaremo
Account properly for no git config
d5bfb67 
The *git.Repo state machine has a state for `RepoNoConfig` (meaning no
repo URL supplied), but hitherto it was not reachable, because the
constructor initialised the state to `RepoNew` (not cloned yet), and
there is no transition from there to `RepoNoConfig`.

We do want to distinguish the "no config" state from just not being
ready, however. This commit makes the state reachable, and accounts
for it in *git.Repo methods.

Ultimately this is visible via the API, where "no config" is now
treated as a reason for ListController results to be read-only.
@squaremo squaremo merged commit 9da7c57 into master Mar 19, 2018
@squaremo squaremo deleted the feature/skipgit branch March 19, 2018 16:41
squaremo added a commit that referenced this pull request Mar 20, 2018
@squaremo
Make upstream.Close() wait for shutdown
a2c82a7 
We let main() exit, and block on graceful shutdown in a `defer`ed
procedure. We _also_ `defer` a call to `upstream.Close()`, which stops
the upstream reconnecting and closes it.

The two deferred procedure calls used to be in such an order that by
coincidence, `upstream.Close()` was _also_ blocked on graceful
shutdown. But this order was shuffled in PR #962.

This commit makes the `upstream.Close()` explicitly wait for shutdown.
This was referenced Mar 20, 2018
Merged
Closed
tamarakaufler pushed a commit that referenced this pull request Apr 3, 2018
@squaremo
Make upstream.Close() wait for shutdown
3a2d2f3 
We let main() exit, and block on graceful shutdown in a `defer`ed
procedure. We _also_ `defer` a call to `upstream.Close()`, which stops
the upstream reconnecting and closes it.

The two deferred procedure calls used to be in such an order that by
coincidence, `upstream.Close()` was _also_ blocked on graceful
shutdown. But this order was shuffled in PR #962.

This commit makes the `upstream.Close()` explicitly wait for shutdown.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rndstr rndstr rndstr left review comments

@samb1729 samb1729 samb1729 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@squaremo @samb1729 @rndstr