fleetd: support dynamic metadata

[dalbani]

Source: #1077

[dalbani]

In relation with the question I asked about a possible TTL feature, I think this structure should be made more future-proof.
In practice, Value should be an object instead of a string.
That gives the room to later implement such improvement for example: https://github.com/dalbani/fleet/commit/c0a305037a214351e62442f7d234865699e9520f#diff-82e778d9c582a0fad1db38076b55deaaR47

[dongsupark]

Build of unit test fails at this line as well as other 4 other call sites, because fakeMachinesSetup() is undefined. In the previous PR #1077 , the function actually did exist.

[dalbani]

Oops, will fix that.

[dalbani]

I've modified api/machines_test.go according to your remark.
Running the test suite locally is now successful.
What is the reason why the CI test triggered by GitHub didn't show up any error by the way?

[dongsupark]

I've modified api/machines_test.go according to your remark.
Running the test suite locally is now successful.
What is the reason why the CI test triggered by GitHub didn't show up any error by the way?

Great. Unit test builds now without error. Thanks!
I'm not sure why the travis job didn't get triggered in the first place. Usually it should run no matter whether there's a build error or not. It was maybe a temporary error from travisci.

[dongsupark]

First of all, thank a lot for the rebase. This helps us a lot go on with the feature.
Is there any chance to write functional tests to cover this feature?

[dalbani]

I have very little experience with Go and fleet, so if you could give me some directions, that would be very useful.

[dongsupark]

@dalbani It's basically about trying to reproduce an exact test scenario inside the fleet's functional test framework, where systemd-nspawn instances run. Normally you could just make use of existing helpers that are already available under the source directory functional. It might be also a good idea to create a sample unit file under functional/fixtures/units.

One of the examples would be a new functional test TestMetadataOperator, which I wrote last week: 68028c7 in #1632. As the original PR didn't include such a test, I needed to write the test on my own. See also https://github.com/coreos/fleet/blob/master/functional/README.md.

[dongsupark]

Gentle ping. Any updates on functional test?
BTW I added this PR to milestone v1.0.0, as it could resolve the issue #555.

[dalbani]

Oh, dear, I had forgotten with the summer and everything :-)
In order to first make my PR in sync with the master branch, I've just committed some RPC-related code.
How important is it to implement the related methods in registry/rpc/rpcregistry.go by the way?
And I've just noticed that the SemaphoreCI build has failed. Do you know why?

[dongsupark]

@dalbani Thanks for pushing the RPC related code. Good point. I think we should also add the MachineMetadata methods to rpc/registry.
As for the semaphoreci failures, every grpc-enabled test is failing. That's interesting. I'd need to have a closer look.

[dongsupark]

@dalbani How about adding into RegistryMux like below:

func (r *RegistryMux) MachineState(machID string) (machine.MachineState, error) {
        return r.getRegistry().MachineState(machID)
}

Wouldn't that resolve the testing error of machine being unable to be found, when enable_grpc=on?

[dongsupark]

I had another look. Unfortunately it turns out, there are still things to be done.
For the non-gRPC path, it looks fine. Functional tests run without errors.
For the gRPC path, necessary methods in RPCRegistry, such as SetMachineMetadata or MachineState, are not implemented. To support the methods, we would need to add also necessary methods into fleet.proto. Then we could support them also in RPCRegistry.
That sounds like a challenging weekend project. ;-)

[dongsupark]

Ah, sorry for noise. On Tuesday I misunderstood something.
We don't have to do extra work in protobuf. Actually we just need to tweak registryMux to make it call methods from etcdRegistry.
So I think we can simply merge this PR, together with an additional functional test. Tomorrow I'll review this PR again, merge it, and I'll create another PR for a simple registryMux fix as well as a functional test for dynamic metadata.

[dalbani]

I am hardly in position to complain, given that I haven't been to the task of writing a functional test :-(
(I have been busy lately with another project even though that's not really an excuse.)
But I have a remark regarding the HTTP API -- well, in fact, I'd like to reiterate the remark that I made above: #1642 (comment)
In type machineMetadataOp struct, I would turn:

    Value     string

into:

    Value     struct {
        Value   string  `json:"value"`
    }

That would keep the ability to enhance the API in the future, for example for a TTL feature mentioned (and proof-of-concept implemented).
Otherwise, with simply Value string, API-breaking changes would be required.

By the way, I have "allowed edits from maintainers" on this PR, should you want to edit it directly.

[dongsupark]

@dalbani All right, I'll do it for avoiding API breakage. Thanks!