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

[full-ci] Store settings via metadata #3232

Merged
merged 51 commits into from
Mar 15, 2022

Conversation

kobergj
Copy link
Collaborator

@kobergj kobergj commented Feb 24, 2022

Description

Encourages settings service to store its data via the metadata service

Related Issue

Motivation and Context

How Has This Been Tested?

  • new unit tests
  • existing acceptance tests cover functionality

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@ownclouders
Copy link
Contributor

ownclouders commented Feb 25, 2022

💥 Acceptance test localApiTests-apiSpaces-ocis failed. Further test are cancelled...

@kobergj kobergj force-pushed the StoreSettingsViaMetadata branch from 3048563 to a873fdc Compare March 1, 2022 09:55
@kobergj kobergj force-pushed the StoreSettingsViaMetadata branch from 1962eeb to 7fcf6f5 Compare March 1, 2022 11:15
@kobergj kobergj force-pushed the StoreSettingsViaMetadata branch from af5c9f5 to f642801 Compare March 1, 2022 13:54
@kobergj kobergj marked this pull request as ready for review March 8, 2022 10:28
@wkloucek wkloucek force-pushed the StoreSettingsViaMetadata branch from fa8364b to 1a9d446 Compare March 11, 2022 15:24
Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

I couldn't get it working. Eg. as admin I am not allowed to add / remove users:
image

When using SETTINGS_STORE_TYPE=filesystem it works

settings/pkg/service/v0/service.go Show resolved Hide resolved
@kobergj
Copy link
Collaborator Author

kobergj commented Mar 14, 2022

@wkloucek Creating users works fine for me. Currently it is necessary to set OCIS_URL envvar because of the IDP of the service user. Could you try if this is the issue?

@C0rby
Copy link
Contributor

C0rby commented Mar 14, 2022

Yeah, it doesn't work on my machine either. I can login but I can't access the settings/accounts UI.
I get the following errors in my log:

2022-03-14T11:50:36+01:00 ERR error initializing metadata client error="error: permission denied: permission denied" service=ocis
2022-03-14T11:50:45+01:00 WRN Failed to execute error="{\"id\":\"com.owncloud.api.thumbnails\",\"code\":404,\"detail\":\"Unsupported file type\",\"status\":\"Not Found\"}" duration=3.591365 method=Thumbnails.GetThumbnail service=thumbnails
2022-03-14T11:51:04+01:00 ERR could not read link, skipping error="readlink /home/corby/.ocis/storage/users/spacetypes/personal/ddc2004c-0977-11eb-9d3f-a793888cd0f8.flock: no such file or directory" match=/home/corby/.ocis/storage/users/spacetypes/personal/ddc2004c-0977-11eb-9d3f-a793888cd0f8.flock pkg=rgrpc service=storage traceid=00000000000000000000000000000000
2022-03-14T11:51:16+01:00 ERR could not read link, skipping error="readlink /home/corby/.ocis/storage/users/spacetypes/personal/ddc2004c-0977-11eb-9d3f-a793888cd0f8.flock: no such file or directory" match=/home/corby/.ocis/storage/users/spacetypes/personal/ddc2004c-0977-11eb-9d3f-a793888cd0f8.flock pkg=rgrpc service=storage traceid=00000000000000000000000000000000
2022-03-14T11:52:15+01:00 ERR error initializing metadata client error="error: not found: stat: error: not found: f1bdd61a-da7c-49fc-8203-0558109d1b4f/settings" service=ocis
2022-03-14T11:52:21+01:00 ERR error initializing metadata client error="error: not found: stat: error: not found: f1bdd61a-da7c-49fc-8203-0558109d1b4f/settings" service=ocis
2022-03-14T11:52:29+01:00 ERR error initializing metadata client error="error: not found: stat: error: not found: f1bdd61a-da7c-49fc-8203-0558109d1b4f/settings" service=ocis

Started oCIS like this: OCIS_URL=https://localhost:9200 OCIS_INSECURE=true PROXY_ENABLE_BASIC_AUTH=true OCIS_LOG_PRETTY=true OCIS_LOG_COLOR=t rue OCIS_LOG_LEVEL=warn bin/ocis server

@kobergj
Copy link
Collaborator Author

kobergj commented Mar 14, 2022

@C0rby this is happening when the settings folder could not be created properly. Could you remove ~/.ocis/storage/metadata/spaces/f1 and try again?

@C0rby
Copy link
Contributor

C0rby commented Mar 14, 2022

Starting with a clean storage and the OCIS_URL works.

@kobergj
Copy link
Collaborator Author

kobergj commented Mar 14, 2022

Ok cool thanks 👍 Imho it is acceptable to have to remove metadata/spaces/f1 if it already exists (it shouldn't normally)

Should we do something about the OCIS_URL? I can hardcode the idp instead of taking it from env...

@kobergj kobergj force-pushed the StoreSettingsViaMetadata branch from e8c251b to 2d4d90a Compare March 14, 2022 12:48
@wkloucek wkloucek dismissed their stale review March 14, 2022 15:31

need to review again

Metadata: config.Metadata{
GatewayAddress: "127.0.0.1:9142",
StorageAddress: "127.0.0.1:9215",
ServiceUserID: "ddc2004c-0977-11eb-9d3f-a793888cd0f8",
Copy link
Contributor

@wkloucek wkloucek Mar 14, 2022

Choose a reason for hiding this comment

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

should we really use the admin user here? why can't we use a system user like for the accounts service (95cb8724-03b2-11eb-a0a6-c33ef8ef53ad)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service user wasn't working properly before as it was creating 10k files on a simple acceptance test. It seems to be fixed though. Let's see what the pipeline says...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well. Seems like it still doesn't work 😞

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wkloucek I found a way around it. Can you check again?

aduffeck and others added 2 commits March 15, 2022 10:07
Adding and removing it again with each ListAccounts() call was a huge
overhead. This is a temporary workaround, the whole service is gonna be
replaced by the idm service soon anyway.
@kobergj kobergj force-pushed the StoreSettingsViaMetadata branch from 6eeed53 to b7c934b Compare March 15, 2022 09:37
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 24 Code Smells

60.5% 60.5% Coverage
9.1% 9.1% Duplication

@kobergj kobergj requested a review from wkloucek March 15, 2022 13:12
@wkloucek wkloucek merged commit 5a67a20 into owncloud:master Mar 15, 2022
ownclouders pushed a commit that referenced this pull request Mar 15, 2022
Merge: b0ea1de d54f75d
Author: Willy Kloucek <[email protected]>
Date:   Tue Mar 15 21:49:58 2022 +0100

    Merge pull request #3232 from kobergj/StoreSettingsViaMetadata

    [full-ci] Store settings via metadata
@kobergj kobergj deleted the StoreSettingsViaMetadata branch March 16, 2022 08:15
@micbar micbar mentioned this pull request Mar 29, 2022
22 tasks
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.

5 participants