Skip to content

HDDS-7248. Recon: Expand the container status page to show all unhealthy container states #3837

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
Merged
smengcl merged 7 commits into from
Oct 21, 2022
Merged

HDDS-7248. Recon: Expand the container status page to show all unhealthy container states #3837

smengcl merged 7 commits into from
Oct 21, 2022

Conversation

@smitajoshi12
Copy link
Contributor

@smitajoshi12 smitajoshi12 commented Oct 14, 2022

What changes were proposed in this pull request?

To show Under Replicated ,Over Replicated,Mis Replicated and All Replicas UnHealthy from Overview and Left Side Panel.
(Please fill in changes proposed in this fix)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7248

How was this patch tested?

Manually
image

@smitajoshi12 smitajoshi12 changed the title HDDS-7248 Recon Expand the container status page to show all unhealth… HDDS-7248. Recon Expand the container status page to show all unhealth… Oct 17, 2022
@smitajoshi12 smitajoshi12 changed the title HDDS-7248. Recon Expand the container status page to show all unhealth… HDDS-7248. Recon: Expand the container status page to show all unhealthy container states Oct 17, 2022
@kerneltime
Copy link
Contributor

kerneltime commented Oct 17, 2022

cc @dombizita

dombizita
dombizita requested changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thank you for the patch @smitajoshi12, this will be a great new feature added to Recon. Currently the ALL_REPLICAS_UNHEALTHY is not populated correctly on the Recon side, so we shouldn't add things related to that as it won't work correctly. I think we should refactor the missing containers part, so we would use that in the same pattern as we are using other unhealthy containers right now (eg. same path and responses). Also we can rename the file from missingContainers.tsx to unhealthyContainer.tsx. Let me know what you think about these two ideas @smengcl

...in/resources/webapps/recon/ozone-recon-web/src/views/missingContainers/missingContainers.tsx Outdated
}
];

const UCOLUMNS = [
Copy link
Contributor

@dombizita dombizita Oct 17, 2022

Choose a reason for hiding this comment

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

I checked and the UCOLUMNS, OCOLUMNS, MCOLUMNS and ACOLUMNS are exactly the same. Why do we have it copied 4 times?
If we want to handle the MISSING state the same as the others we could have only the COLUMNS (starts on line 97) and adjust it to the parameters.

Copy link
Contributor Author

@smitajoshi12 smitajoshi12 Oct 18, 2022

Choose a reason for hiding this comment

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

I have used another REPLICATEDCOLUMNS as one columns differs Unhealthy Since.

image

Copy link
Contributor

@dombizita dombizita Oct 19, 2022

Choose a reason for hiding this comment

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

We can use the Unhealthy Since column for the MISSING state as well.

Copy link
Contributor Author

@smitajoshi12 smitajoshi12 Oct 20, 2022

Choose a reason for hiding this comment

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

Done Code Changes

smengcl
smengcl reviewed Oct 18, 2022
View reviewed changes
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12 for the patch. Overall looks good. I have posted some comments above.

smitajoshi12 and others added 2 commits October 19, 2022 18:56
@smitajoshi12
HDDS-748 Recon: Expand the container status page to show all unhealth…
75dc72f 
…y container states Review Changes
@smengcl
Use /containers/unhealthy/MISSING instead of containers/missing
23de1ec 
Change-Id: I9eff56d24bce5884cbc77cca67e840b6330aa546
smengcl
smengcl reviewed Oct 19, 2022
View reviewed changes
...in/resources/webapps/recon/ozone-recon-web/src/views/missingContainers/missingContainers.tsx
Comment on lines +305 to +309
axios.all([
axios.get('/api/v1/containers/unhealthy/MISSING'),
axios.get('/api/v1/containers/unhealthy/UNDER_REPLICATED'),
axios.get('/api/v1/containers/unhealthy/OVER_REPLICATED'),
axios.get('/api/v1/containers/unhealthy/MIS_REPLICATED')
Copy link
Contributor

@smengcl smengcl Oct 19, 2022

Choose a reason for hiding this comment

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

Let's try to get all unhealthy container information at once with:

Suggested change
axios.all([
axios.get('/api/v1/containers/unhealthy/MISSING'),
axios.get('/api/v1/containers/unhealthy/UNDER_REPLICATED'),
axios.get('/api/v1/containers/unhealthy/OVER_REPLICATED'),
axios.get('/api/v1/containers/unhealthy/MIS_REPLICATED')
axios.all([
axios.get('/api/v1/containers/unhealthy')

then change the logic accordingly. Hopefully this will be a small change.

Copy link
Contributor Author

@smitajoshi12 smitajoshi12 Oct 20, 2022

Choose a reason for hiding this comment

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

Getting this Response.

{"missingCount":0,"underReplicatedCount":0,"overReplicatedCount":0,"misReplicatedCount":0,"containers":[]} but not sure How will be Containers data and states so Can we change this things in next JIRA I am not sure about changes and logic unless I get proper data from cluster.

Copy link
Contributor

@dombizita dombizita Oct 20, 2022

Choose a reason for hiding this comment

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

In what kind of cluster did you get this response? Did you change everything accordingly to this change? If you don't pass any state as a parameter to the endpoint it will return all the containers (javadoc on the @Path("/unhealthy/{state}"): Passing null returns all containers.) . Based on the javadoc on the @Path("/unhealthy"):

   * Return
   * {@link org.apache.hadoop.ozone.recon.api.types.UnhealthyContainerMetadata}
   * for all unhealthy containers.

I am fine if we ask every states endpoint individually for now, but please change the MISSING state ask accordingly to the others (/api/v1/containers/unhealthy/MISSING).

Copy link
Contributor

@smengcl smengcl Oct 21, 2022

Choose a reason for hiding this comment

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

Can we change this things in next JIRA I am not sure about changes and logic unless I get proper data from cluster.

Alright. Let's move the refactoring work to a follow-up JIRA.

smengcl
smengcl reviewed Oct 19, 2022
View reviewed changes
...in/resources/webapps/recon/ozone-recon-web/src/views/missingContainers/missingContainers.tsx
const missingContainers: IMissingContainerResponse[] = missingContainersResponse.containers;

axios.all([
axios.get('/api/v1/containers/unhealthy/MISSING'),
Copy link
Contributor

@smengcl smengcl Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TODO: Try with docker compose cluster response.

Copy link
Contributor Author

@smitajoshi12 smitajoshi12 Oct 20, 2022

Choose a reason for hiding this comment

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

Tried with Docker Cluster Response but not getting actual response.

http://localhost:9888/api/v1/containers/unhealthy/MISSING

{"missingCount":0,"underReplicatedCount":1,"overReplicatedCount":0,"misReplicatedCount":0,"containers":[]}

Copy link
Contributor

@dombizita dombizita Oct 20, 2022

Choose a reason for hiding this comment

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

What kind of response did you expect from the endpoint? What is the state of the cluster? If we don't have missing containers in the cluster this is a good response, this is the scheme we are looking for.

smengcl and others added 2 commits October 19, 2022 09:35
@smengcl
Fix db.json response for missing containers.
3304b56 
Change-Id: I3488d231c80d42e15a469d10df414a6f59dfb841
@smitajoshi12
HDDS-7248 Recon: Expand the container status page to show all unhealt…
73d7d40 
…hy container states Review Comments
smengcl
smengcl approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

+1

@smitajoshi12 Please open a new JIRA for the refactoring (that combines 4 axios gets into one. ref)

@smengcl smengcl merged commit ae59f8a into apache:master Oct 21, 2022
errose28 added a commit to errose28/ozone that referenced this pull request Oct 26, 2022
@errose28
Merge branch 'master' into HDDS-6447
24ee2fa 
* master: (718 commits)
  HDDS-7342. Move encryption-related code from MultipartCryptoKeyInputStream to OzoneCryptoInputStream (apache#3852)
  HDDS-7413. Fix logging while marking container state unhealthy (apache#3887)
  Revert "HDDS-7253. Fix exception when '/' in key name (apache#3774)"
  HDDS-7396. Force close non-RATIS containers in ReplicationManager (apache#3877)
  HDDS-7121. Support namespace summaries (du, dist & counts) for legacy FS buckets (apache#3746)
  HDDS-7258. Cleanup the allocated but uncommitted blocks (apache#3778)
  HDDS-7381. Cleanup of VolumeManagerImpl (apache#3873)
  HDDS-7253. Fix exception when '/' in key name (apache#3774)
  HDDS-7182. Add property to control RocksDB max open files (apache#3843)
  HDDS-7284. JVM crash for rocksdb for read/write after close (apache#3801)
  HDDS-7368. [Multi-Tenant] Add Volume Existence check in preExecute for OMTenantCreateRequest (apache#3869)
  HDDS-7403. README Security Improvement (apache#3879)
  HDDS-7199. Implement new mix workload Read/Write Freon command (apache#3872)
  HDDS-7248. Recon: Expand the container status page to show all unhealthy container states (apache#3837)
  HDDS-7141. Recon: Improve Disk Usage Page (apache#3789)
  HDDS-7369. Fix wrong order of command arguments in Nonrolling-Upgrade.md (apache#3866)
  HDDS-6210. EC: Add EC metrics (apache#3851)
  HDDS-7355. non-primordial scm fail to get signed cert from primordial SCM when converting an unsecure cluster to secure (apache#3859)
  HDDS-7356. Update SCM-HA.zh.md to match the English version (apache#3861)
  HDDS-6930. SCM,OM,RECON should not print ERROR and exit with code 1 on successful shutdown (apache#3848)
  ...

Conflicts:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestLegacyReplicationManager.java
@smitajoshi12 smitajoshi12 deleted the HDDS-7248 branch October 27, 2022 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dombizita dombizita dombizita requested changes

@smengcl smengcl smengcl approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smitajoshi12 @kerneltime @smengcl @dombizita