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

Let fluxd answer queries without a git repo supplied #962

Merged
merged 4 commits into from Mar 19, 2018

Conversation

@squaremo
Copy link
Member

commented Feb 26, 2018

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 force-pushed the feature/skipgit branch from 109f8fa to 5728904 Mar 2, 2018
@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 force-pushed the feature/skipgit branch from 5728904 to 77467b2 Mar 8, 2018
@squaremo squaremo requested a review from sambooo Mar 8, 2018
@squaremo squaremo force-pushed the feature/skipgit branch from 77467b2 to 1b4dfff Mar 9, 2018
Copy link
Contributor

left a comment

Few nitpicks, bit of code praise, some questions.

"email", *gitEmail,
"sync-tag", *gitSyncTag,
"notes-ref", *gitNotesRef,
"set-author", *gitSetAuthor)

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Nitpick: put the closing bracket on the next line - no reason yanking this line should take more keystrokes :)

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

Under Pike's Eye.

EventWriter: eventWriter,
Logger: log.With(logger, "component", "daemon"), LoopVars: &daemon.LoopVars{
Logger: log.With(logger, "component", "daemon"),
LoopVars: &daemon.LoopVars{

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Did gofmt align this line like this?

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

Yup. Odd isn't it -- if I move the fields to the same line, it aligns it with the others, but when they're on their own lines, it puts it there.

@@ -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 has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Kubernetes

// 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 has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Is this intentionally unused at the moment, but written into the ListServices code as a sort of docs for possible future behaviour? Cool! Seems like a sensible property of a Controller 👍

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

Yeah; it would be filled in by the cluster/kubernetes code, but that currently omits the only things that would have it set (which are add-ons).

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

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

What's the motivation behind making this omitempty but none of the other properties?

It's plausible that a Controller could have no Policies, or maybe even no Containers, just as it may not be ReadOnly.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

There's no especial reason, and in fact I think this is the wrong way around for this particular field -- I want a missing value (e.g., from an old daemon) to look like an empty string, for API consumers.

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 12, 2018

Contributor

With or without omitempty, unmarshalling with a missing value will result in an empty string in the consumer. Only way to distinguish missing versus empty is to make this field a pointer.

I suppose if we care about saving a few bytes we can pepper our structs with omitempty and hope any other languages consuming our api Do The Right Thing.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

unmarshalling with a missing value will result in an empty string in the consumer.

From Go, yes. But not from say, JavaScript.

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 has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

I like the use of the bare switch statements here and below - keeps all the references to each new bit of state easily within a screenful. Should be easier to read and follow than some of our bigger methods.

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 has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Should be readOnly

@@ -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 has been minimized.

Copy link
@sambooo

sambooo Mar 11, 2018

Contributor

Should this be used somewhere? Since no code references this value, this PR doesn't

Let fluxd answer queries without a git repo supplied,

so much as it does

Let fluxd answer queries before the supplied git repo is ready,

right?

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 12, 2018

Author Member

Since no code references this value

https://github.com/weaveworks/flux/blob/master/git/repo.go#L156

but .. I take your general point, which is that this case isn't handled (or isn't handled well). Possibly there should be another ReadOnlyReason for "No git repo supplied", to distinguish it from "git repo isn't ready".

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 13, 2018

Author Member

Oh I see, that state is not reachable, because the git.Repo is initialised in the state RepoNew and there are no transitions into RepoNoConfig.

OK, I have fixed that (most recent commit). If the git.Repo is given an empty origin URL, it is initialised in RepoNoConfig. This percolates out to the API, where it's represented as const ReadOnlyNoRepo ReadOnlyReason = "NoRepo".

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 14, 2018

Contributor

there are no transitions into RepoNoConfig

I worded my comment poorly enough that it wasn't even obvious to me this is what I actually meant... Yep, there was no code which assigned that value to anything.

If you're going to link code you should use the permalink. You never know how much time you might save the next code archeologist 😄.

This comment has been minimized.

Copy link
@rndstr

rndstr Mar 14, 2018

Contributor

If you're going to link code you should use the permalink

y is a nice shortcut for this when looking at a branch HEAD

@sambooo

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2018

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 Feb 26, 2018
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.
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.
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 squaremo force-pushed the feature/skipgit branch from 1b4dfff to 8f8dc0c Mar 12, 2018
@squaremo

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

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.

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
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@squaremo squaremo deleted the feature/skipgit branch Mar 19, 2018
squaremo added a commit that referenced this pull request Mar 20, 2018
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.
tamarakaufler pushed a commit that referenced this pull request Apr 3, 2018
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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.