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

Add CRUD support for availability zone #1049

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

joseph-v
Copy link
Collaborator

What this PR does / why we need it:
This PR adds Create/Update/List/Delete operations on availability zone so that management of zone is more flexible.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1040

Special notes for your reviewer: Implementation is in progress

Release note:

@codecov
Copy link

codecov bot commented Nov 13, 2019

Codecov Report

Merging #1049 into development will increase coverage by 0.1%.
The diff coverage is 42.35%.

@@              Coverage Diff               @@
##           development    #1049     +/-   ##
==============================================
+ Coverage        34.58%   34.69%   +0.1%     
==============================================
  Files               97      100      +3     
  Lines            17849    18165    +316     
==============================================
+ Hits              6173     6302    +129     
- Misses           10804    10967    +163     
- Partials           872      896     +24
Impacted Files Coverage Δ
pkg/api/controllers/pool.go 55.55% <ø> (+6.53%) ⬆️
client/client.go 28.07% <0%> (-0.51%) ⬇️
pkg/utils/urls/urls.go 33.33% <0%> (-1.67%) ⬇️
pkg/db/drivers/etcd/etcd.go 25.2% <0%> (-1.24%) ⬇️
osdsctl/cli/cli.go 22% <100%> (+1.59%) ⬆️
pkg/dock/discovery/discovery.go 33.67% <29.41%> (-0.41%) ⬇️
pkg/api/controllers/zone.go 53.84% <53.84%> (ø)
client/fake.go 47.96% <54.54%> (+0.96%) ⬆️
osdsctl/cli/zone.go 56.45% <56.45%> (ø)
client/zone.go 63.82% <63.82%> (ø)
... and 3 more

Copy link
Collaborator

@wisererik wisererik left a comment

Choose a reason for hiding this comment

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

  1. please use AvailabilityZon instead of Zon.
  2. it's will have some impact of pool dicovery. When discovering pool, Dock daemon will check whether its availability zone exists in the system.

// assertTestResult(t, w.Code, 200)
// assertTestResult(t, output, SampleAvailabilityZones)
// })
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove unused code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

// "strings"

// log "github.com/golang/glog"
"github.com/opensds/opensds/pkg/api/policy"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

"github.com/opensds/opensds/pkg/model"
// "github.com/opensds/opensds/pkg/utils/constants"
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


// An OpenSDS zone is identified by a unique name and ID.
type ZoneSpec struct {
*BaseModel
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use AvailabilityZoneSpec

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@joseph-v joseph-v force-pushed the availability_zone branch 6 times, most recently from aaedd1f to 189aa20 Compare November 16, 2019 06:55
@joseph-v joseph-v changed the title [WIP] Add CRUD support for availability zone Add CRUD support for availability zone Nov 16, 2019
@joseph-v joseph-v changed the base branch from master to development November 20, 2019 04:43
Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

LGTM

Url: urls.GenerateZoneURL(urls.Etcd, "", zoneID),
}
dbRes := c.Get(dbReq)
if dbRes.Status != "Success" {
Copy link
Contributor

Choose a reason for hiding this comment

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

what will be the behaviour if c.Get() returns null or errornous code?

}
dbRes := c.Get(dbReq)
if dbRes.Status != "Success" {
log.Error("When get zone in db:", dbRes.Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

message should start with lower case.
please also change the below line nos with lower case
line no: 1348, 1385, 1418, 1431, 1441

azExists := false
azs, err := dr.c.ListAvailabilityZones(ctx)
if err != nil {
log.Errorf("When list zone %s in db: %v\n", pol.AvailabilityZone, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. And please take a look of all files about lower case starting letter

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.

None yet

4 participants