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

adds spec for status endpoint #122

Merged
merged 15 commits into from Dec 12, 2018

Conversation

Projects
None yet
6 participants
@xh3b4sd
Copy link
Member

xh3b4sd commented Dec 4, 2018

Please make sure in case you are changing the spec all our customers are informed beforehand.

You can throw a message in #sig-customer with the changes and the Solution Engineers will take care of
sharing the changes with the customers.

Towards giantswarm/giantswarm#2206.

@xh3b4sd xh3b4sd self-assigned this Dec 4, 2018

@xh3b4sd xh3b4sd requested review from giantswarm/sig-ux Dec 4, 2018

Show resolved Hide resolved definitions.yaml Outdated
Show resolved Hide resolved definitions.yaml Outdated
@teemow

This comment has been minimized.

Copy link
Member

teemow commented Dec 4, 2018

Sweet!

@marians FYI we will stick with the exact structure of the custom resource here. this interface might not (yet) be perfect but proposed changes should go into apiextensions and not here anymore.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 4, 2018

Thanks for the info!

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 4, 2018

Please understand that this is tough for me now. I guess I already know why you're proposing it the way you do, anyway I have to point out the problems in the context of the API.

image

Why have a cluster key and a aws key on the top level?

This is the cluster status endpoint. Having it contain a cluster key on the top level "stutters", as golint would say.

If we would copy the design we have in the cluster detials endpoint we would take the keys that are now in the cluster object and would move them to the top. Then we would also have a provider-specific key like aws, azure, kvm.

Why have an availabilityZones key under aws?

In the cluster creation request and cluster details response, the availabiltiy_zones key is a cluster-level attribute, not provider specific.

And there is the camelCase vs. underscore_case style difference.

I know this stems from the desire to simply take parts from two CRs mostly unmodified and publish them. We can do that, but it doesn't make for a great API design and it will definitely raise some people's eyebrows.

xh3b4sd added some commits Dec 4, 2018

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 4, 2018

The only reason here is the quick fix in order to make progress. I explained that already in the SIG UX sync. We talked about that. I changed the camel cases to underscore cases. My fried brain was slow here. The rest of your concerns, as valid as they might be, should be addressed and improved later. Having this feedback and generating a vision for these things is good. But we will certainly not wait for it to be perfect in order to finish the autoscaling story.

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 4, 2018

I would maybe also question how useful the "last heartbeat time" is and if we want to expose or just drop it. It is cool somehow, but I do not see the immediate value. Filtering out properties can be easily done. Rewriting the whole thing should maybe be tried to be prevented, depending on the necessary overhead required.

@teemow

This comment has been minimized.

Copy link
Member

teemow commented Dec 4, 2018

@marians this is why i mentioned discussions whould go to apiextensions and not here. Feel free to open an issue or pull request there. this here is not the right place.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 4, 2018

Tim and I spoke. We decided to make it very clear and obvious that the ../status/ endpoint really just passes through what the CR's status contains. This includes the camelCase, and resources. This way we can also point users to apiextensions for further docs.

@teemow We spoke about this topic in SIG UX today with a slightly different outcome (not copying 100% what we have in apiextensions). Now it wasn't exactly clear to me that the result of exposing the CR's status would mean what we have now (especially aws.availabilityZones, cluster.*).

We can favour agility over stability and consistency here, but we should try to manage user's expectations. I offered Tim to provide a preamble text for this endpoint to help with that.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 4, 2018

Description/disclaimer added in df532b4

I don't know yet what would be the best link targets to let people find more info, especially about the status part.

@teemow

This comment has been minimized.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 4, 2018

Very good, thanks! Links added.

This endpoint passes through the status content of the Kubernetes resources representing
a cluster in the according custom resource. That is, depending on the provider:
- [awsconfig.provider.giantswarm.io](https://godoc.org/github.com/giantswarm/apiextensions/pkg/apis/provider/v1alpha1#AWSConfig)

This comment has been minimized.

@teemow

This comment has been minimized.

@marians

marians Dec 4, 2018

Member

I didn't do it now, because I think it's better to point to the CR we mention. The users will find their way there though.

@tuommaki
Copy link

tuommaki left a comment

In general this LGTM. Couple proposals for alternative wording. Use of period in the end of the description sentences was varying so perhaps making that consistent?

Show resolved Hide resolved definitions.yaml Outdated
Show resolved Hide resolved definitions.yaml Outdated
Show resolved Hide resolved definitions.yaml Outdated
Show resolved Hide resolved definitions.yaml Outdated
Show resolved Hide resolved spec.yaml Outdated
Show resolved Hide resolved spec.yaml Outdated

tuommaki and others added some commits Dec 5, 2018

Update definitions.yaml
Co-Authored-By: xh3b4sd <xh3b4sd@gmail.com>
Update definitions.yaml
Co-Authored-By: xh3b4sd <xh3b4sd@gmail.com>
Update spec.yaml
Co-Authored-By: xh3b4sd <xh3b4sd@gmail.com>
Update spec.yaml
Co-Authored-By: xh3b4sd <xh3b4sd@gmail.com>
@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 10, 2018

Is this spec fine for you? Please let me know so we can start the implementation.

@teemow

teemow approved these changes Dec 10, 2018

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 10, 2018

I am still not sure whether it is the right way to include the status model into the spec, because of the version-based changes we are expecting there. We could basically only support one version and would let all other versions fail. I'd like to hear @oponder's opinion.

An alternative would be to specify the entire model for that endpoint freely using additionalProperties: true (https://swagger.io/docs/specification/data-models/dictionaries/#free-form).

This would mean that clients using the spec could use the generated function to pull the status for a cluster, including authentication, but would not get support for parsing the info returned. This leaves the work to deal with schema changes between releases to the client implementations.

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 10, 2018

The status structure here is a union of all providers. Certain fields will be empty. I do not see the problem here.

@teemow

This comment has been minimized.

Copy link
Member

teemow commented Dec 11, 2018

@marians if i understand correctly you are worried because the schema of this endpoint is based on the version of the cluster, correct? but we will only use the latest apiextensions and therefore fields will only be empty like @xh3b4sd said. so the only thing we need to take care of in the future is to adapt this schema here everytime we change the status resource in apiextensions.

@oponder

This comment has been minimized.

Copy link
Member

oponder commented Dec 11, 2018

I'm not sure about the versioning situation, if it works as timo says, then I have no problems here.

We have to make sure to make backwards compatible changes when making new versions of the CR in apiextensions, or otherwise increment the GS API version, though chances are it won't come to that, and the GS API will be deprecated by then.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 11, 2018

As discussed in the SIG-UX meeting we'd like to make it clear that the apiextensions repo is the source of truth for the schema of the status endpoint. And avoid duplicating the schema definition. So here we will try using only additionalProperties: true in the status model definition.

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 11, 2018

This is how it looks like now. The example is empty, which is odd. Users have to click the links to understand that.

screenshot 2018-12-11 at 15 27 21

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 11, 2018

If we want to, we can add an example via responses -> examples -> application/json. But this is something that has to be maintained, too.

You find plenty of examples for this in spec.yaml, e. g. in getClusters.

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 11, 2018

There was a spec. There was an example. We said we do not want to maintain in two places. So I removed both. When we keep the example we should keep the spec. As I said, I do not mind which way to go, but mixing this seems to be super wrong to me.

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 11, 2018

Cool, then leave it out. You stated you found something odd. I spelled out what I think the options are.

@xh3b4sd

This comment has been minimized.

Copy link
Member Author

xh3b4sd commented Dec 12, 2018

The api PR got merged. The status endpoint is implemented. Can we merge this here then? Is Marian happy? And everybody else?

@teemow

This comment has been minimized.

Copy link
Member

teemow commented Dec 12, 2018

LGTM

@MarcelMue
Copy link
Member

MarcelMue left a comment

LGTM

@xh3b4sd xh3b4sd merged commit a581bae into master Dec 12, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@xh3b4sd xh3b4sd deleted the spec-status-endpoint branch Dec 12, 2018

@marians

This comment has been minimized.

Copy link
Member

marians commented Dec 13, 2018

Cool! Here is a real-world example from Azure

{
  "aws": {
    "availabilityZones": null
  },
  "cluster": {
    "conditions": [
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "status": "True",
        "type": "Updated"
      }
    ],
    "network": {
      "cidr": ""
    },
    "nodes": [
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-master-000000",
        "version": "2.0.1"
      },
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-worker-000002",
        "version": "2.0.1"
      },
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-worker-000003",
        "version": "2.0.1"
      },
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-worker-000004",
        "version": "2.0.1"
      },
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-worker-000009",
        "version": "2.0.1"
      },
      {
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "name": "6iec4-worker-00000e",
        "version": "2.0.1"
      }
    ],
    "resources": [
      {
        "conditions": [
          {
            "lastHeartbeatTime": "0001-01-01T00:00:00Z",
            "lastTransitionTime": "0001-01-01T00:00:00Z",
            "status": "InstancesUpgrading",
            "type": "Stage"
          }
        ],
        "name": "instancev4"
      }
    ],
    "scaling": {
      "desiredCapacity": 0
    },
    "versions": [
      {
        "date": "2018-11-15T13:37:39.863296744Z",
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "semver": "2.0.0"
      },
      {
        "date": "2018-12-06T11:22:15.474857203Z",
        "lastHeartbeatTime": "0001-01-01T00:00:00Z",
        "lastTransitionTime": "0001-01-01T00:00:00Z",
        "semver": "2.0.1"
      }
    ]
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment