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

CNF-15570: Add new resource servers args and update some utils #379

Merged

Conversation

alegacy
Copy link
Collaborator

@alegacy alegacy commented Dec 5, 2024

This adds command line argument handling for the new resource server, and updates some utilities to simplify how they are invoked by converting optional arguments to variadic arguments.

The PR is broken down into a few commits to separate chunks for functionality.

@alegacy alegacy requested a review from mlguerrero12 December 5, 2024 16:31
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 5, 2024

@alegacy: This pull request references CNF-15570 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

This adds the first pass at implementing the data collector for the inventory data. This initial implementation synchronizes the resource pool, resource type, and resource objects from the ACM and creates corresponding data change event records to track when any of these objects are created or updated. A future commit will address the delete use case as well as the other objects (e.g., deployment manager).

The PR is broken down into a few commits to separate chunks for functionality.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.


// CompareObjects compares two objects and returns the tags for fields having differing values. Any field names listed
// in `excluded` are ignored.
func CompareObjects[T db.Model](a, b T, excluded ...string) DBTag {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see where this function is used in subsequent commits. I have many concerns/questions there.
I think we are abusing of reflect and generics. I see that you use it to update only some fields but I wonder if it is worth the trouble. As far as I know, postgres is a mvcc db which means an update is an insert-delete underneath. I know, there is some gain by not sending everything, but is it really too much? How often do you expect to have changes.

And if performance is a concern here, then I see you are using the slowest approach, updating an object at a time. CopyFrom is the fastest, then single query with multiple rows, after that batch (multiiple queries inside) and finally one row at a time. jackc/pgx#1545

Also, postgres has an upsert verb that can be used instead of checking in code if an insert or update is needed.

I'm almost done reviewing the first 3 commits of this PR. I suggest you split this pr in 2, one with the first 3 commits so that we can merge them today. The last 3 will require more time. If you do that, please remove this function from the "CNF-15570: Add support for updating only some fields" commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't anticipate that the volume of updates will be high. I was following best practices of only updating necessary fields. This is what most ORMs do to avoid overloading the database with updating unnecessary fields. We can certainly remove that, but from a debug point of view it makes it difficult to tell why something is being updated if everything is being written on all updates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should qualify my comment about the volume of updates. As the code is written now every record will get updated to set the generation id to track stale records. I could switch to a different approach to manage that and we wouldn't need as many updates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mlguerrero12 as per your request I have split the PR. The other one with the top 3 commits (plus a new one for the subscription notification handling) is over here.

@alegacy alegacy force-pushed the feature/add-inventory-collect branch 2 times, most recently from 8442d96 to 8a0272b Compare December 7, 2024 01:41
@alegacy
Copy link
Collaborator Author

alegacy commented Dec 8, 2024

/hold

would like to merge this manually without squashing

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2024
Copy link
Contributor

@mlguerrero12 mlguerrero12 left a comment

Choose a reason for hiding this comment

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

Just minor comments, looks good. Please change the title and description of the PR

internal/service/resources/cmd/serve.go Show resolved Hide resolved

// The O-Cloud ID and External address arguments are mandatory while all other arguments are optional. The Global
// O-Cloud ID is special in that it is not strictly mandatory to start the server but it is mandatory to enable
// some API endpoints (e.g., subscriptions).
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed to enable some API endpoints?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the global cloud id is a required piece of data to send notification events to subscribers and so I don't believe we should allow a new subscription to be created until the end user has provided us with our global cloud id. Our servers need to be up and running before the user will have provided this so we can't make it mandatory. The best we can do is block some API endpoints if it is not provided -- with a 400 bad request with a suitable message.

It appears in both the alarm event and the cloud available notification events. I think I originally confused the cloud available event with the regular inventory change notifications so it isn't in fact needed on the resource server API endpoints, but it is necessary on the alarm server subscription create API. ...but I'm sort of the view that if we block the alarm subscriptions we should block the inventory subscriptions as well to create consistent behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do resource and alarms servers need to up and running before the user has provided the global cloud id? Even after they do, we will need to restart the servers, or not?

How is this (global cloud id) provided? (haven't read the code for that part yet) Shouldn't be mandatory by the operator?

internal/service/resources/cmd/serve.go Outdated Show resolved Hide resolved
internal/service/resources/api/openapi.yaml Outdated Show resolved Hide resolved
Adds the command line arguments required by the resource server to
achieve parity with the previous implementation of the resource server.

Signed-off-by: Allain Legacy <[email protected]>
This makes changes to the Update utility so that it is possible to
request that only some fields be set in the update operation.  This is
to facilitate changes required by the upcoming resource data collector
functionality that will be synchronizing data between ACM and our
persistent storage.

Signed-off-by: Allain Legacy <[email protected]>
Use a variadic argument for field names rather than an array to avoid
needing to pass in `nil` when not required.

Signed-off-by: Allain Legacy <[email protected]>
@alegacy alegacy force-pushed the feature/add-inventory-collect branch from 8a0272b to bdf29f0 Compare December 9, 2024 12:51
@alegacy alegacy changed the title CNF-15570: Add inventory data collector/persistence CNF-15570: Add new resource servers args and update some utils Dec 9, 2024
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Dec 9, 2024

@alegacy: This pull request references CNF-15570 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

This adds command line argument handling for the new resource server, and updates some utilities to simplify how they are invoked by converting optional arguments to variadic arguments.

The PR is broken down into a few commits to separate chunks for functionality.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@alegacy
Copy link
Collaborator Author

alegacy commented Dec 9, 2024

Just minor comments, looks good. Please change the title and description of the PR

done

@mlguerrero12
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2024
Copy link

openshift-ci bot commented Dec 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlguerrero12

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2024
@alegacy
Copy link
Collaborator Author

alegacy commented Dec 9, 2024

/label tide/merge-method-rebase

@openshift-ci openshift-ci bot added the tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges. label Dec 9, 2024
@alegacy
Copy link
Collaborator Author

alegacy commented Dec 9, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d4afb17 into openshift-kni:main Dec 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants