Skip to content

配置结构体深拷贝bug修复以及修复baremetal agent注册bug#19285

Closed
nyl1001 wants to merge 0 commit intoyunionio:masterfrom
nyl1001:master
Closed

配置结构体深拷贝bug修复以及修复baremetal agent注册bug#19285
nyl1001 wants to merge 0 commit intoyunionio:masterfrom
nyl1001:master

Conversation

@nyl1001
Copy link
Copy Markdown

@nyl1001 nyl1001 commented Jan 17, 2024

What this PR does / why we need it:

Does this PR need to be backport to the previous release branch?:

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 17, 2024

比如修改keystone服务configmap中如下默认配置参数
max_group_roles_in_project: 20
max_user_roles_in_project: 20
修改为:
max_group_roles_in_project: 200
max_user_roles_in_project: 200
重修发布keystone服务后,在keystone服务中的配置MaxUserRolesInProject依然为默认值20,而不是修改后的200。
新加配置也不生效,只能得到默认值。

@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 18, 2024

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 18, 2024

https://www.cloudpods.org/docs/guides/auth_security/serviceconfig/#%E6%9C%8D%E5%8A%A1%E9%85%8D%E7%BD%AE
这个地方的代码昨天都review过了,并不是被数据库中覆盖了,数据库没有的配置也被重置成默认值了,你们原先的那个拷贝函数使用不当

@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 18, 2024

https://www.cloudpods.org/docs/guides/auth_security/serviceconfig/#%E6%9C%8D%E5%8A%A1%E9%85%8D%E7%BD%AE
这个地方的代码昨天都review过了,并不是被数据库中覆盖了,数据库没有的配置也被重置成默认值了,你们原先的那个拷贝函数使用不当

climc service-config-edit 修改的配置优先级最高,也是推荐使用的改配置的方式

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 18, 2024

https://www.cloudpods.org/docs/guides/auth_security/serviceconfig/#%E6%9C%8D%E5%8A%A1%E9%85%8D%E7%BD%AE
这个地方的代码昨天都review过了,并不是被数据库中覆盖了,数据库没有的配置也被重置成默认值了,你们原先的那个拷贝函数使用不当

climc service-config-edit 修改的配置优先级最高,也是推荐使用的改配置的方式

还没涉及到修改,你这边初始化就有问题了

@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 18, 2024

climc service-config keystone max_group_roles_in_project=200 会先改数据库中 keystone.whitelisted_config表里面的配置,然后会通知相应的服务动态更改配置,下次服务重启会优先选用service-config里面的配置,你可以改下试试,然后看下数据库中的配置,再加下日志打印下配置

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 19, 2024

climc service-config keystone max_group_roles_in_project=200 会先改数据库中 keystone.whitelisted_config表里面的配置,然后会通知相应的服务动态更改配置,下次服务重启会优先选用service-config里面的配置,你可以改下试试,然后看下数据库中的配置,再加下日志打印下配置
哎,你这些不用跟我强调,我都给你说过,这里的代码我都看过,我感觉你都不明白我的意思。
你初始化配置的时候使用的copyOptions有bug,我指出这个问题,你却谈后面的whitelist补救,这两者之间有关系吗?存在冲突吗?当然whitelist若存在,会覆盖存在的配置。但不存在的呢?
我始终强调初始化配置时的copyOptions函数有bug,这个不能完成深拷贝,我纠正这个问题。

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 19, 2024

climc service-config keystone max_group_roles_in_project=200 会先改数据库中 keystone.whitelisted_config表里面的配置,然后会通知相应的服务动态更改配置,下次服务重启会优先选用service-config里面的配置,你可以改下试试,然后看下数据库中的配置,再加下日志打印下配置

哎,你这些不用跟我强调,我都给你说过,这里的代码我都看过,我感觉你都不明白我的意思。
你初始化配置的时候使用的copyOptions有bug,我指出这个问题,你却谈后面的whitelist补救,这两者之间有关系吗?存在冲突吗?当然whitelist若存在,会覆盖存在的配置。但不存在的呢?
我始终强调初始化配置时的copyOptions函数有bug,这个不能完成深拷贝,我纠正这个问题。

@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 19, 2024

ServiceBlacklistOptionMap = map[string][]string{
configmap里面只有改 ServiceBlacklistOptionMap 里面配置的字段才会生效,本身就是这么设计的

@swordqiu
Copy link
Copy Markdown
Member

加了一个单元测试,实现应该是没问题的:https://github.com/yunionio/cloudpods/pull/19312/files

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

反复强调,深拷贝有bug,本身这个修改也是针对这个深拷贝的,我一开始就把用例解释得很清楚了,你们要这么拧巴,随你们吧,当我没提

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

加了一个单元测试,实现应该是没问题的:https://github.com/yunionio/cloudpods/pull/19312/files

你这用例不够,无法验证出问题,你用实际的keystone的configmap配置作为用例试试吧,深拷贝没有你这么使用的

@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 22, 2024

加了一个单元测试,实现应该是没问题的:https://github.com/yunionio/cloudpods/pull/19312/files

你这用例不够,无法验证出问题,你用实际的keystone的configmap配置作为用例试试吧,深拷贝没有你这么使用的

我试过你的改动,没有起到作用,改完configmap后,重启服务配置依然是20,本身设计就是需要用climc去改配置,不然删除configmap后会重置配置,没办法持久化到数据库中

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

Collaborator

还在扯其他的,你的这个拷贝函数是不是有bug?首先初始化要保证正确,修改配置用climc没有问题,也不冲突?

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

加了一个单元测试,实现应该是没问题的:https://github.com/yunionio/cloudpods/pull/19312/files

你这用例不够,无法验证出问题,你用实际的keystone的configmap配置作为用例试试吧,深拷贝没有你这么使用的

我试过你的改动,没有起到作用,改完configmap后,重启服务配置依然是20,本身设计就是需要用climc去改配置,不然删除configmap后会重置配置,没办法持久化到数据库中

没起作用的原因就是copyOptions不是深拷贝,明白吗?修改配置用climc没有问题,没有反对你这个

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

加了一个单元测试,实现应该是没问题的:https://github.com/yunionio/cloudpods/pull/19312/files

你这用例不够,无法验证出问题,你用实际的keystone的configmap配置作为用例试试吧,深拷贝没有你这么使用的

我试过你的改动,没有起到作用,改完configmap后,重启服务配置依然是20,本身设计就是需要用climc去改配置,不然删除configmap后会重置配置,没办法持久化到数据库中

没起作用的原因就是copyOptions不是深拷贝,明白吗?修改配置用climc没有问题,没有反对你这个。你也别谈你的设计就是故意让这个copyOptions不能完成深拷贝,故意让他不起作用的哈,那我真就呵呵了

@nyl1001
Copy link
Copy Markdown
Author

nyl1001 commented Jan 22, 2024

你用我这个改动后的函数,就可以正确完成copyOptions,这个我是在我们这边的生产环境验证了的,不然也不会在这里跟你们费劲的解释了

@nyl1001 nyl1001 changed the title 配置结构体深拷贝bug修复 配置结构体深拷贝bug修复以及修复baremetal agent注册bug Jan 24, 2024
@ioito
Copy link
Copy Markdown
Collaborator

ioito commented Jan 24, 2024

你用我这个改动后的函数,就可以正确完成copyOptions,这个我是在我们这边的生产环境验证了的,不然也不会在这里跟你们费劲的解释了

目前参数的改法是有歧义,这些参数将会在configmap中移除 yunionio/cloudpods-operator#1042 ,只能通过climc service-config-edit 去更改,另外一个commit可以单独提pr合并

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.

3 participants