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

WIP: Add plugin API #99

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

jhernand
Copy link
Collaborator

This patch adds the definition of the Kubernetes custom resource definitions that will be used by the O2 IMS implementation to interact with the hardware manager.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2024
@openshift-ci openshift-ci bot requested review from danielerez and irinamihai May 27, 2024 07:57
Copy link

openshift-ci bot commented May 27, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jhernand. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


// BMC contains the details needed to connect to the baseboard management controller of a node.
type BMC struct {
URL string `json:"url"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a reminder, we probably want a reference to the creds secret here.
E.g.
CredentialsSecretName string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I added that in the latest version of the patch.

// FirmwareSettings contains the firmware settings for the nodes that use this profile.
//
// +kubebuilder:validation:Optional
FirmwareSettings map[string]string `json:"firmwareSettings,omitempty"`
Copy link
Collaborator

@danielerez danielerez May 27, 2024

Choose a reason for hiding this comment

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

IIRC, there was a requirement to support separate extensions maps for FirmwareSettings and FirmwareVersions (?)
I.e. we can consider having these as structs instead of maps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I think I will remove these fields in favor of the extensions. Vendors can there add whatever they need. For example:

extensions:
- oran.acme.com/firmwareSettings: |
    {
       "CStates": "C6",
       "VirtualizationEnabled": "true"
    }
- oran.acme.com/firmwareVersions: |
    {
      "UEFI": "1.3",
      "NIC": "2.7"
    }

In the future if we see that different vendors use the same data structures we can re-add these fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, sounds like a good idea. Just probably worth to document that example for the keys then.

const (
// OrderFulfilledCondition is the condition that indicates that an order has been
// completely and successfully completed.
OrderFulfilledCondition = "Fullfilled"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: 'Fulfilled'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Irinal, will do.

// Location is the geographical location of the requested deployment manager.
//
// +kubebuilder:validation:Required
Location string `json:"profile"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, shouldn't the golang variable and the json key have the same name, location / profile in this case? Or is the difference here intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is a plain mistake, I will fix the JSON key.

// deployment manager.
//
// +kubebuilder:validation:Required
Template string `json:"template,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need omitempty if the field is Required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, will remove it.

metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

// NodeProfiles is a collection of named node profiles that will be ued in the template.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: used

Extensions map[string]string `json:"extensions,omitempty"`
}

// DeploymentManagerTemplateNodeProfile associates a name with a set of node node settings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra node


// DeploymentManager is the schema a deployment manager.
//
// +kubebuilder:resource:shortName=dm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe dmgr would be a bit more descriptive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is more descriptive, but it would make other short names longer and harder to type, for example dmgrtemplate instead of dmtemplate. I will keep this for now, unless you have a strong objection.


// BMCDetails contains the details needed to connect to the baseboard management controller of
// a node.
type BMCDetails struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this structure used anywhere. Is it for future use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is used in the next version of the patch.

@jhernand jhernand force-pushed the add_plugin_api branch 2 times, most recently from 4151eaa to 46e91ef Compare May 29, 2024 10:35
This patch adds the definition of the Kubernetes custom resource
definitions that will be used by the O2 IMS implementation to interact
with the hardware manager.

Signed-off-by: Juan Hernandez <[email protected]>
@jhernand jhernand force-pushed the add_plugin_api branch 2 times, most recently from e99c09b to 5ea8ef0 Compare May 29, 2024 10:46
@jhernand jhernand merged commit 3498522 into openshift-kni:main May 29, 2024
7 of 8 checks passed
@jhernand jhernand deleted the add_plugin_api branch May 29, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants