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.

Sign up for GitHub

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

Jump to bottom

#1119 disable updating feature #1126

Merged
merged 9 commits into from
Oct 18, 2022
Merged

#1119 disable updating feature #1126

merged 9 commits into from
Oct 18, 2022

Conversation

IvanPleshkov
Copy link
Contributor

@IvanPleshkov IvanPleshkov commented Oct 13, 2022 •
edited
Loading

#1119
This PR implements disabling updates. Cloud team can forbidden any updates from users to prevent large disk usage.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally using cargo fmt command prior to submission?
  3. Have you checked your code using cargo clippy command?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@generall
Copy link
Member

Could you please add a possibility to configure explanation message into API, as described in the task

generall
generall requested changes Oct 13, 2022
View reviewed changes
openapi/openapi-service.ytt.yaml
get:
summary: Collect telemetry data
description: Collect telemetry data including app info, system info, collections info, cluster info, configs and statistics
operationId: telemetry
Copy link
Member

Choose a reason for hiding this comment

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

could you please add tags, so the API is grouped in UI properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/actix/api/service_api.rs Outdated
pub enable: bool,
}

#[put("/enable_updating")]
Copy link
Member

Choose a reason for hiding this comment

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

In REST naming convention it is prefered to use noun + (optional) action for API endpoints. For example:

GET /is_updating_enabled -> GET /lock
PUT /enable_updating -> PUT /lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, I use write_lock name instead of lock

openapi/openapi-service.ytt.yaml Outdated
type: boolean
responses: #@ response(array(reference("TelemetryData")))

/enable_updating:
Copy link
Member

Choose a reason for hiding this comment

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

Is there supposed to be a param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@IvanPleshkov IvanPleshkov requested a review from generall October 14, 2022 07:03
agourlay
agourlay reviewed Oct 14, 2022
View reviewed changes
src/common/helpers.rs
#[derive(Deserialize, Serialize, JsonSchema)]
pub struct WriteLockStatus {
pub locked: bool,
pub error_message: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: this object mixes both locking and unlocking state together.

What about having two REST APIs:

  • lock with optional message
  • unlock (no message required)

Copy link
Contributor Author

@IvanPleshkov IvanPleshkov Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. Do you like such API?

PUT /wtite_lock -> lock writing with optional message
PUT /write_unlock -> unlock writing
GET /write_lock -> check if locked
GET /write_unlock -> check if unlocked

Copy link
Member

Choose a reason for hiding this comment

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

Good for me!

Maybe you could ask for some feedback from the cloud team if they are the main users for 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.

I implemented it. I will fix API if the cloud team will wish something else.

Copy link
Member

Choose a reason for hiding this comment

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

GET /write_lock -> check if locked
GET /write_unlock -> check if unlocked

How is this supposed to work? Look redundant

Copy link
Member

Choose a reason for hiding this comment

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

also lock is supposed to be a noun, not verb. So I would say that PUT /write_unlock is redundant too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check if locked and check if unlocked. @agourlay sold me ideas about PUT write_lock and PUT write_unlock because lock and unlock have different parameters. And GET function imho is expected in this both paths

Copy link
Member

@agourlay agourlay Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What about going away from this idea of exposing a write lock which is a bit confusing in terms of naming.

For instance, I find having a read-only mode clearer.

GET /read-only
PUT /read-only/enable 
PUT /read-only/disable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the option, LGTM. Do you accept such api @generall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Final api

POST /locks
{
  error_message: "Ram high watermark reached"
  write: true
}

GET is the same

agourlay
agourlay reviewed Oct 14, 2022
View reviewed changes
lib/storage/src/content_manager/toc.rs Outdated
pub fn set_write_lock(&self, locked: bool, error_message: Option<String>) -> bool {
let prev_value = self
.write_lock
.swap(locked, std::sync::atomic::Ordering::Relaxed);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: it would nice to capture the date at which the lock was enabled.

This would enable to have a since field in the error message by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO Not needed. Because this since cloud team can format itself in the error message. Only they know about the reasons for blocking and only they should decide about since timestamp

@IvanPleshkov IvanPleshkov requested a review from agourlay October 18, 2022 08:42
@generall generall merged commit f50f1f4 into dev Oct 18, 2022
@IvanPleshkov IvanPleshkov deleted the disable-updating-feature branch October 18, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@generall generall generall approved these changes

@agourlay agourlay Awaiting requested review from agourlay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IvanPleshkov @generall @agourlay