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

F2f refactor #13

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

F2f refactor #13

wants to merge 6 commits into from

Conversation

csams
Copy link
Collaborator

@csams csams commented May 24, 2024

Updates per the F2F

csams added 6 commits May 23, 2024 11:45
Signed-off-by: Christopher Sams <csams@redhat.com>
Make Resource a distinct resource type that's disconnected from specific
resource types other than by convention.

Signed-off-by: Christopher Sams <csams@redhat.com>
Signed-off-by: Christopher Sams <csams@redhat.com>
Signed-off-by: Christopher Sams <csams@redhat.com>
Signed-off-by: Christopher Sams <csams@redhat.com>
Signed-off-by: Christopher Sams <csams@redhat.com>
@randymgeorge
Copy link

randymgeorge commented May 28, 2024

For POST of a k8scluster:

  • I thought that we were going to include ResourceType in request body and support POSTing to the .../resources URL. This enables extensibility. Otherwise, we need code support for another endpoint.
  • Href, CreatedAt, LastUpdatedAt should not be part of the POST.
  • Should "Displayname" not be "DisplayName"?
  • It should be stated that Workspace is optional and if not provided, will be added to the default workspace
  • Should KubeVendor be an enum?
  • There is no mention of using the "ExternalClusterID" as the resource/rest ID
  • The description for ExternalClusterID is OCP specific and the resource type is k8s (generic)
  • The credential reference type should include things like hashivault, etc.
  • enums should have an "unknown" value
  • CloudProviderID was to be a "name" and not an "id". With that said, looking at telemetry it is referred to as "cloud_type" with the following enum values:
    platform_type
    VSphere (IPI)
    VSphere (UPI)
    BareMetal (IPI)
    BareMetal (UPI)
    AWS (IPI)
    AWS (UPI)
    Azure (IPI)
    Azure (UPI)
    IBMCloud (IPI)
    IBMCloud (UPI)
    OpenStack (IPI)
    OpenStack (UPI)
    GCP (IPI)
    GCP (UPI)
    Nutanix (IPI)
    Nutanix (UPI)
    Virt (IPI)
    Unknown (IPI)
    None (UPI)


servers:
- url: https://console.redhat.com

paths:
/api/inventory/v1.0/resources:
description: Base path for common inventory
description: Retrieve a heterogenous list of Resources
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this just return what resource types are being managed by the inventory - and not the instances itself ? The instance list could be very large and unless we allow robust filtering it may not be that useful. In that case the minimal filtering should be labels, resource_types and perhaps some attributes even. (example clusters and rhel machines-that-has-gpu for dev env)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like you have not focused on the filters yet - so then you can ignore that part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I have comments below about querying and how that needs to be figured out.

- $ref: '#/components/schemas/RootPolicyDetail'
- $ref: '#/components/schemas/IntermediatePolicyDetail'

K8sCluster:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets populate a real example - if it makese sense. Would be happy to provide real data for this if it helps

schema:
$ref: "#/components/schemas/K8sCluster"
responses:
"200":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this return 200 or 202?
What is the thought process -

  1. if we are going to accept the request and then process async, it better be 202. We really cannot generate a CI-cluster-id yet, can we?
  2. Also if ACS and ACM (just making it up) tries to POST on same cluster at different times, we will not be creating 2 different resources, will we?

@bjoydeep
Copy link
Collaborator

I apologize for using the term intermediate-policy. We should remove this. This is in a reality an internal implementation detail which in theory ACM could change it.

  1. We have a policy. Calling it root probably makes sense.
  2. Then that policy is distributed to a set of clusters.
  3. And of course we do this for many policies.
  4. As you can imagine each instances of a root policy foo that is running on cluster-1, cluster-4 and cluster-5 may have different status-es. So we need to be able to update that.

Queries usually are -

  1. give me all policies (we really are talking about root policies) on a cluster/s
  2. give me all clusters that have a policy foo
  3. give me all policies on all clusters (this can be a large on - will cross check if this is really needed in the first iteration)

@randymgeorge
Copy link

On a PUT, the following attributes should not be allowed to change:

  • KubeVendor
  • CloudProvider
  • ExternalClusterID
  • Any of the Reporter data

What is the diff between ExternalClusterID and ResourceIDAlias?

How is the reporter identified, e.g. if ACS posted about cluster Foo and ACM posted about cluster Foo? i see "type" under reporter in the GET, but not in the POST.

@clyang82
Copy link
Collaborator

Do we need to consider store the status/state in common inventory? I think we should.

  • the status can be jsonb format. so that the different reporter can report different status they can recognize. I am not sure if it can be too large. maybe we need to control the size.
  • the state can be avaible/unavaible/etc.

schema:
$ref: "#/components/schemas/Error"

/api/inventory/v1.0/resources/k8s-clusters/{id}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to v1.0/v1.1? maybe just v1/v2

in: path
schema:
type: string
required: true
description: |-
The resource ID assigned to the resource by the inventory.
A reporter alias may also be used as a resource ID using the format:
A reporter alias may also be used as a resource ID using the format:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate how to generate the instanceId for the reporter? the same reporter may report multiple resources. how to ensure the instanceId is the same for the same reporter? do we store it somewhere?

@bjoydeep
Copy link
Collaborator

What identifies a cluster uniquely. Posing this question given that we may have to disambiguate (between 2 different tools reporting data about the same cluster). Unless we have a unique fingerprint that detects a cluster (kind of natural primary key), how can we do this?

@randymgeorge
Copy link

@bjoydeep Great point. This needs to be explicitly rendered in the model. For k8sclusters, I think that we said, when kubeVendor = OpenShift, then id would be UUID. When kubeVendor=EKS, it would be the cluster ARN, etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants