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

MGMT-14704: Provide info on custom/vs non custom manifest in manifest endpoint. #5278

Merged
merged 1 commit into from Jul 3, 2023

Conversation

paul-maidment
Copy link
Contributor

It has been reported that users have no way to determine whether or not a manifest is a custom manifest or one generated by the assisted-service. This PR introduces a change that uses the file system to store additional metadata about a manifest to indicate whether or not the manifest is a custom one.

For example, if the original file is stored in

"e09df13f-6f31-42c2-8361-2b5605f80e77/manifests/openshift/99-openshift-machineconfig-master-kargs.yaml"

Then, if an only if the manifest is custom - a corresponding metadata file will be created in the following path.

"e09df13f-6f31-42c2-8361-2b5605f80e77/manifest-attributes/openshift/99-openshift-machineconfig-master-kargs.yaml/user-defined"

Any internally generated manifests are considered to be "non custom" and are created as such. All other manifests are considered to be custom and will follow the scheme above.

When the user retrieves the manifest list, an additional parameter will be supplied for each manifest to indicate the custom/non custom status of the manifest.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
    Building a custom version of the service, creating a cluster and uploading manifests
    Fetching the manifest list
    Observing that behaviour is as expected.
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 5, 2023

@paul-maidment: This pull request references MGMT-14704 which is a valid jira issue.

In response to this:

It has been reported that users have no way to determine whether or not a manifest is a custom manifest or one generated by the assisted-service. This PR introduces a change that uses the file system to store additional metadata about a manifest to indicate whether or not the manifest is a custom one.

For example, if the original file is stored in

"e09df13f-6f31-42c2-8361-2b5605f80e77/manifests/openshift/99-openshift-machineconfig-master-kargs.yaml"

Then, if an only if the manifest is custom - a corresponding metadata file will be created in the following path.

"e09df13f-6f31-42c2-8361-2b5605f80e77/manifest-attributes/openshift/99-openshift-machineconfig-master-kargs.yaml/user-defined"

Any internally generated manifests are considered to be "non custom" and are created as such. All other manifests are considered to be custom and will follow the scheme above.

When the user retrieves the manifest list, an additional parameter will be supplied for each manifest to indicate the custom/non custom status of the manifest.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
    Building a custom version of the service, creating a cluster and uploading manifests
    Fetching the manifest list
    Observing that behaviour is as expected.
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

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 kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 5, 2023
@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 Jun 5, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jun 5, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 5, 2023
@paul-maidment paul-maidment force-pushed the MGMT-14704 branch 3 times, most recently from ffeeccf to f8cde49 Compare June 7, 2023 09:16
@paul-maidment paul-maidment marked this pull request as ready for review June 7, 2023 09:20
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 7, 2023
@openshift-ci openshift-ci bot requested review from romfreiman and tsorya June 7, 2023 09:21
@filanov
Copy link
Contributor

filanov commented Jun 7, 2023

/assign @carbonin
/assign @danielerez

swagger.yaml Outdated Show resolved Hide resolved
@omertuc
Copy link
Contributor

omertuc commented Jun 7, 2023

Was this also tested with the service running with our S3 filesystem driver? When running as our online SaaS we store files in S3, not a filesystem, and I think this PR calls for some testing of that before we merge it.

Alternatively, you can wait for it to go through integration/staging and then test it and hope it works before it reaches prod

@@ -1415,7 +1415,7 @@ func (m *Manager) deleteClusterFiles(ctx context.Context, c *common.Cluster, obj
var failedToDelete []string
for _, file := range files {
//skip log and manifests deletion when deleting cluster files
if folder == "" && (strings.Contains(file, "logs") || strings.Contains(file, "manifests")) {
if folder == "" && (strings.Contains(file, "logs") || strings.Contains(file, "manifests") || strings.Contains(file, "manifest-attributes")) {
Copy link
Contributor

@omertuc omertuc Jun 7, 2023

Choose a reason for hiding this comment

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

Now that we can tell apart user manifests from our own service manifests, I think it's best that this function deletes the service manifests.

The original motivation for this skip was that we didn't want to destroy user uploaded manifests when the user resets their installation, but preserving the service generated ones is useless, and was just an unfortunate side-effect of protecting the user manifests. The service ones would anyway get regenerated when the user hits install again, there's no point in protecting them. If anything, clearing those manifests I think this might solve some edge case bugs I can think of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a ticket for this as I think it's not the primary focus of this ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please link the ticket once you created it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #5278 (cc79b03) into master (d2a504a) will increase coverage by 0.05%.
The diff coverage is 66.10%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5278      +/-   ##
==========================================
+ Coverage   67.48%   67.54%   +0.05%     
==========================================
  Files         221      226       +5     
  Lines       33042    33272     +230     
==========================================
+ Hits        22300    22475     +175     
- Misses       8729     8773      +44     
- Partials     2013     2024      +11     
Impacted Files Coverage Δ
internal/cluster/cluster.go 66.44% <33.33%> (-0.07%) ⬇️
internal/manifests/manifests.go 70.46% <64.44%> (-1.63%) ⬇️
internal/bminventory/inventory.go 69.85% <100.00%> (-0.01%) ⬇️
...oller/controllers/clusterdeployments_controller.go 71.91% <100.00%> (+0.04%) ⬆️
internal/manifests/manifests_v2.go 91.66% <100.00%> (ø)
internal/network/manifests_generator.go 75.74% <100.00%> (-0.82%) ⬇️
internal/operators/manager.go 77.64% <100.00%> (ø)

... and 20 files with indirect coverage changes

@paul-maidment paul-maidment force-pushed the MGMT-14704 branch 3 times, most recently from b979dea to 6cad9da Compare June 12, 2023 09:06
@paul-maidment
Copy link
Contributor Author

Was this also tested with the service running with our S3 filesystem driver? When running as our online SaaS we store files in S3, not a filesystem, and I think this PR calls for some testing of that before we merge it.

Alternatively, you can wait for it to go through integration/staging and then test it and hope it works before it reaches prod

I'll test this in integration and take action if needed. I have not used any new API's so am not expecting problems in this area.

@filanov
Copy link
Contributor

filanov commented Jun 14, 2023

/assign @carbonin

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Do you think we could create a subsystem test to cover cases with generated manifests or even manifests that don't have the metadata file?

@@ -1156,7 +1156,8 @@ func (r *ClusterDeploymentsReconciler) syncManifests(ctx context.Context, log lo
Content: swag.String(base64.StdEncoding.EncodeToString([]byte(manifest))),
FileName: swag.String(filename),
Folder: swag.String(models.ManifestFolderOpenshift),
}})
}},
true)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a type for this with some kind of string enum so it's more clear from the caller what this means.

That would also give us the flexibility to reference more than two options here.

@@ -34,6 +34,7 @@ var _ manifestsapi.ManifestsAPI = &Manifests{}

// ManifestFolder represents the manifests folder on s3 per cluster
const ManifestFolder = "manifests"
const ManifestMetadataFolder = "manifest-attributes"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use this in the delete cluster files function

func (m *Manifests) CreateClusterManifestInternal(ctx context.Context, params operations.V2CreateClusterManifestParams) (*models.Manifest, error) {
const (
ManifestSourceCustomManifest string = "custom-manifest"
ManifestSourceGenerated string = "generated"
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty much what I was talking about in the bool -> enum comment. You just need a type definition like we do for auth here

swagger.yaml Outdated
@@ -6655,6 +6655,10 @@ definitions:
file_name:
type: string
description: The file name prefaced by the folder that contains it.
is_custom_manifest:
description: Indicates whether this is a manifest that was manually uploaded by the user, as opposed to being internally generated by the Assisted Service
type: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the best way to do this would be to define this as a string enum.

Then swagger will create the constants for you and you can use them in the function calls (and instead of defining them yourself).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we return manifests that the user didn't generate at all? As a user, I expect to see only the manifests I created.

Copy link
Contributor

@omertuc omertuc Jun 22, 2023

Choose a reason for hiding this comment

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

I wondered the same - the reason is explained in the ticket - see the screenshot in that ticket:

image

Copy link
Member

Choose a reason for hiding this comment

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

I think @avishayt asked the correct question in the ticket.

The distinction is if we want to communicate to the user where all these manifests are coming from or if we only care about showing them the ones they created. If it's the former I think a string enum is the choice, but if we only need to know if we're showing them or not then using a bool internally (not in the API) makes more sense to me.


func (m *Manifests) createMetadataForManifest(ctx context.Context, clusterID strfmt.UUID, path string, isCustomManifest bool) error {
log := logutil.FromContext(ctx, m.log)
log.Infof("Is custom manifest %t", isCustomManifest)
Copy link
Member

Choose a reason for hiding this comment

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

This message could be improved or removed.

log.Infof("Done deleting cluster manifest %s for cluster %s", path, params.ClusterID.String())
return nil
}

func (m *Manifests) UpdateClusterManifestInternal(ctx context.Context, params operations.V2UpdateClusterManifestParams) (*models.Manifest, error) {
m.log.Infof("Call to update cluster manifest internal")
Copy link
Member

Choose a reason for hiding this comment

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

Clean up these logs, I don't think we need this.

Comment on lines 243 to 222
m.log.Infof("srcFolder: %s, srcFileName: %s, srcPath: %s", srcFolder, srcFileName, srcPath)
m.log.Infof("destFolder: %s, destFileName: %s, destPath: %s", destFolder, destFileName, destPath)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we can remove these debug-looking logs.

Comment on lines 285 to 254
isCustomManifest := m.isCustomManifest(ctx, params.ClusterID.String(), srcFolder, srcFileName)
if isCustomManifest != nil {
err = m.createMetadataForManifest(ctx, params.ClusterID, destPath, *isCustomManifest)
if err != nil {
return nil, err
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we only need this if src and dest path are different.

@@ -202,16 +282,30 @@ func (m *Manifests) UpdateClusterManifestInternal(ctx context.Context, params op
return nil, err
}

isCustomManifest := m.isCustomManifest(ctx, params.ClusterID.String(), srcFolder, srcFileName)
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that a user can update a generated manifest?

If not do we really need this?

Copy link
Member

Choose a reason for hiding this comment

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

And even if a user updates a generated manifest, does that not make it a user manifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is really an opportunity to do this as manifest generation is one of the last actions before install starts, as part of the preparation phase (I think.)
This would present no opportunity for the user to override this.

if srcPath != destPath {
m.log.Infof("Removing old manifest %s for cluster %s as the new file is at %s", srcPath, params.ClusterID.String(), destPath)
err = m.deleteManifest(ctx, params.ClusterID, srcPath)
if err != nil {
return nil, err
}
if isCustomManifest != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a check for nil or some value?

@paul-maidment paul-maidment force-pushed the MGMT-14704 branch 12 times, most recently from b6f96c3 to ee64fc0 Compare June 25, 2023 16:15
@paul-maidment
Copy link
Contributor Author

/retest

@@ -1975,7 +1975,7 @@ var _ = Describe("cluster reconcile", func() {
mockInstallerInternal.EXPECT().HostWithCollectedLogsExists(gomock.Any()).Return(false, nil)
mockInstallerInternal.EXPECT().GetKnownHostApprovedCounts(gomock.Any()).Return(5, 5, nil).Times(2)
mockManifestsApi.EXPECT().ListClusterManifestsInternal(gomock.Any(), gomock.Any()).Return(models.ListManifests{}, nil).Times(1)
mockManifestsApi.EXPECT().CreateClusterManifestInternal(gomock.Any(), gomock.Any()).Return(nil, errors.Errorf("error")).Times(1)
mockManifestsApi.EXPECT().CreateClusterManifestInternal(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.Errorf("error")).Times(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

true instead of gomock.Any() - for all three affected tests?

manifestMetadataPrefix := filepath.Join(clusterID, constants.ManifestMetadataFolder)
isCustomManifest, err := m.objectHandler.DoesObjectExist(ctx, filepath.Join(manifestMetadataPrefix, folder, fileName, constants.ManifestSourceUserSupplied))
if err != nil {
m.log.Warnf("Could not determine if manifest %s/%s is a %s manifest due to error %s, leaving this as nil", folder, fileName, constants.ManifestSourceUserSupplied, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nobody will ever see this message... Why not propagate the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested, I will propagate this error and make it visible by making it fatal.
This might lead to more actual failures when fetching manifests but will at least make those errors visible.

return isCustomManifest
}

func (m *Manifests) deleteMetadataForManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (m *Manifests) deleteMetadataForManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {
func (m *Manifests) unmarkUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {

Copy link
Contributor

Choose a reason for hiding this comment

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

Because in the method name you call it "metadata", but in reality you're only dealing with "user supplied".

return nil
}

func (m *Manifests) createMetadataForManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (m *Manifests) createMetadataForManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {
func (m *Manifests) markUserSuppliedManifest(ctx context.Context, clusterID strfmt.UUID, path string) error {

Comment on lines 104 to 105
manifestMetadataPrefix := filepath.Join(clusterID, constants.ManifestMetadataFolder)
isCustomManifest, err := m.objectHandler.DoesObjectExist(ctx, filepath.Join(manifestMetadataPrefix, folder, fileName, constants.ManifestSourceUserSupplied))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use GetManifestMetadataObjectName()?

… endpoint.

It has been reported that users have no way to determine whether or not a manifest is a custom manifest or one generated by the assisted-service.
This PR introduces a change that uses the file system to store additional metadata about a manifest to indicate whether or not the manifest is a custom one.

For example, if the original file is stored in

```
"e09df13f-6f31-42c2-8361-2b5605f80e77/manifests/openshift/99-openshift-machineconfig-master-kargs.yaml"
```

Then, if an only if the manifest is custom -  a corresponding metadata file will be created in the following path.

```
"e09df13f-6f31-42c2-8361-2b5605f80e77/manifest-attributes/openshift/99-openshift-machineconfig-master-kargs.yaml/user-defined"
```

Any internally generated manifests are considered to be "non custom" and are created as such.
All other manifests are considered to be custom and will follow the scheme above.

When the user retrieves the manifest list, an additional parameter will be supplied for each manifest to indicate the custom/non custom status of the manifest.
@@ -1149,14 +1149,16 @@ func (r *ClusterDeploymentsReconciler) syncManifests(ctx context.Context, log lo

// create/update all manifests provided by configmap data
for filename, manifest := range manifestsFromConfigMap {
is_user_manifest := true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_user_manifest := true
isUserManifest := true

manifests = append(manifests, &models.Manifest{FileName: filepath.Join(parts[3:]...), Folder: parts[2]})
fileName := filepath.Join(parts[3:]...)
folder := parts[2]
err, is_user_manifest := m.isUserManifest(ctx, params.ClusterID, folder, fileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err, is_user_manifest := m.isUserManifest(ctx, params.ClusterID, folder, fileName)
err, isUserManifest := m.isUserManifest(ctx, params.ClusterID, folder, fileName)

@avishayt
Copy link
Contributor

avishayt commented Jul 3, 2023

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Jul 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avishayt, paul-maidment

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:
  • OWNERS [avishayt,paul-maidment]

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 lgtm Indicates that a PR is ready to be merged. label Jul 3, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 3, 2023

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 689a73c into openshift:master Jul 3, 2023
19 checks passed
danielerez pushed a commit to danielerez/assisted-service that referenced this pull request Oct 15, 2023
… endpoint. (openshift#5278)

It has been reported that users have no way to determine whether or not a manifest is a custom manifest or one generated by the assisted-service.
This PR introduces a change that uses the file system to store additional metadata about a manifest to indicate whether or not the manifest is a custom one.

For example, if the original file is stored in

```
"e09df13f-6f31-42c2-8361-2b5605f80e77/manifests/openshift/99-openshift-machineconfig-master-kargs.yaml"
```

Then, if an only if the manifest is custom -  a corresponding metadata file will be created in the following path.

```
"e09df13f-6f31-42c2-8361-2b5605f80e77/manifest-attributes/openshift/99-openshift-machineconfig-master-kargs.yaml/user-defined"
```

Any internally generated manifests are considered to be "non custom" and are created as such.
All other manifests are considered to be custom and will follow the scheme above.

When the user retrieves the manifest list, an additional parameter will be supplied for each manifest to indicate the custom/non custom status of the manifest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants