-
Notifications
You must be signed in to change notification settings - Fork 32
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
v3 support #121
v3 support #121
Conversation
Dear @porscheme Thanks. |
err := metaClient.Disconnect() | ||
if err != nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log err or just _ = metaClient.Disconnect()
?
pkg/nebula/meta_client.go
Outdated
} | ||
|
||
func (m *metaClient) Balance(req *meta.BalanceReq) (*meta.BalanceResp, error) { | ||
return m.client.Balance(req) | ||
func (m *metaClient) Balance(req *meta.AdminJobReq) (*meta.AdminJobResp, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this func because we don't recommend use it, user can use BalanceData or BalanceLeader instead
Cmd: meta.AdminCmd_LEADER_BALANCE, | ||
Paras: [][]byte{space}, | ||
} | ||
resp, err := m.client.RunAdminJob(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use m.balance(req)
?
pkg/nebula/meta_client.go
Outdated
log.Info("balance plan running now") | ||
return m.BalanceStatus(resp.Id) | ||
log.Info("balance job running now") | ||
return m.BalanceStatus(*resp.GetResult_().JobID, space) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems balanceStatus is a long task, do we need use async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this's not a ideal implementation. I will save the last job in status field.
pkg/nebula/meta_client.go
Outdated
} | ||
resp, err := m.client.RunAdminJob(req) | ||
if err != nil { | ||
return false, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if meta done or leader changed? balanceStatus will never stop?
pkg/util/retry/retry.go
Outdated
return wait.PollImmediateUntil(interval, func() (bool, error) { | ||
done, err := fn(ctx) | ||
if err != nil { | ||
if done { | ||
return false, err | ||
} | ||
} else if done { | ||
return true, nil | ||
} | ||
return false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about
done, err := fn(ctx)
if err != nil {
return false, err
}
return done, nil
Workload WorkloadStatus `json:"workload,omitempty"` | ||
Version string `json:"version,omitempty"` | ||
Phase ComponentPhase `json:"phase,omitempty"` | ||
HostsAdded bool `json:"hostsAdded,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HostsAdded
only for Storage
?
oldReplicas := extender.GetReplicas(oldWorkload) | ||
newReplicas := extender.GetReplicas(newWorkload) | ||
if !nc.Status.Storaged.HostsAdded || *newReplicas > *oldReplicas { | ||
if err := c.addStorageHosts(nc, *oldReplicas, *newReplicas); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not error when the same host add more than once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here only executed once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
addStorageHosts
successfully.syncComponentStatus
failed.- reconcile, and call
addStorageHosts
again.
if err := c.addStorageHosts(nc, *oldReplicas, *newReplicas); err != nil { | ||
_, ok := err.(*net.DNSError) | ||
if ok { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It's sync successfully when DNSError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here capture DNSError will not block subsequent reconcile logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it's the DNSError
a normal behavior?
If all the following succeed, then add host succeeded?
empty := len(spaces) == 0 | ||
if !empty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if len(spaces) > 0
?
if len(hosts) > 0 { | ||
if err := metaClient.RemoveHost(hosts); err != nil { | ||
return err | ||
if !empty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if len(spaces) > 0
?
@@ -26,7 +26,8 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime/schema" | |||
"sigs.k8s.io/controller-runtime/pkg/client" | |||
|
|||
ng "github.com/vesoft-inc/nebula-go/v2/nebula" | |||
ng "github.com/vesoft-inc/nebula-go/v3/nebula" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about nebulago
to unify the alias.
pkg/util/retry/retry.go
Outdated
var stop <-chan struct{} | ||
if ctx != nil { | ||
stop = ctx.Done() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it block whenstop
is nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done may return nil if this context never be canceled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How use ctx.Done()
directly, ctx
generally cannot be nil.
tests/e2e/nebulacluster/util.go
Outdated
@@ -39,7 +39,7 @@ import ( | |||
"k8s.io/utils/pointer" | |||
"sigs.k8s.io/controller-runtime/pkg/client" | |||
|
|||
nebula "github.com/vesoft-inc/nebula-go/v2" | |||
nebula "github.com/vesoft-inc/nebula-go/v3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about nebulago to unify the alias.
err := metaClient.Disconnect() | ||
if err != nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ = metaClient.Disconnect()
?
How do you think about it when it's needed multiple versions at the same kubernetes? |
2. add balance job status
@MegaByte875 @wey-gu how I test this? |
@MegaByte875 could you help @porscheme with a step by step guide? |
I will update helm charts later, you can deploy nebula v3 by helm @porscheme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
upgrade nebula client
update apis/template configuration