On 03.03.2016 18:38, Martin Basti wrote:


On 02.03.2016 13:47, Oleg Fayans wrote:
Hi Martin,

I've made the requested changes.

The full set of necessary patches is attached.


On 03/02/2016 10:05 AM, Martin Basti wrote:

On 02.03.2016 00:12, Oleg Fayans wrote:
Hi Martin,

On 03/01/2016 07:04 PM, Martin Basti wrote:
On 01.03.2016 14:56, Martin Basti wrote:

On 01.03.2016 12:37, Martin Basti wrote:
On 01.03.2016 12:32, Martin Basti wrote:
On 29.02.2016 13:16, Oleg Fayans wrote:
Hi all,

Finally the tests pass.

The patch 0024 applies on top of patch 0022 (please, consider
reviewing
it also). Besides, the whole functionality depends on Martin's
patch N 0421

All patches pass pylint.
hello,

I cannot apply patches on master branch
Martin^2
My bad I applied wrong patch

On 12/19/2015 11:56 PM, Martin Basti wrote:
On 17.12.2015 10:04, Oleg Fayans wrote:
Hi Martin,

I am sorry, in my previous email I attached the old version of
patch
0016. The correct on is attached.

On 12/16/2015 05:47 PM, Martin Basti wrote:
On 16.12.2015 15:39, Martin Basti wrote:
On 15.12.2015 10:29, Oleg Fayans wrote:
Hi Martin,

The updated patches are attached. Patch 0017 includes all
changes from
patch 0018, so, if you approve this one, there would be no
need to
continue with the review of 0018. This one contains all changes
related
to you remarks from 0018 review. Please see my explanation
on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this
bug:
https://fedorahosted.org/freeipa/ticket/5550

On 12/09/2015 12:17 PM, Martin Basti wrote:
On 09.12.2015 12:10, Martin Basti wrote:
On 09.12.2015 11:14, Oleg Fayans wrote:
Hi Martin

On 12/09/2015 10:30 AM, Martin Basti wrote:
On 08.12.2015 23:48, Oleg Fayans wrote:
Substituted a hardcoded suffix name with a constant
DOMAIN_SUFFIX_NAME

On 12/08/2015 02:33 PM, Oleg Fayans wrote:
Hi all,


The patches are rebased against the current master.

On 12/02/2015 05:10 PM, Martin Basti wrote:
On 02.12.2015 16:18, Oleg Fayans wrote:
Hi Martin,

On 12/01/2015 04:08 PM, Martin Basti wrote:
On 27.11.2015 16:26, Oleg Fayans wrote:
And patch N 16 passes lint too:

On 11/27/2015 04:03 PM, Oleg Fayans wrote:
Hi,

On 11/27/2015 03:26 PM, Martin Basti wrote:
On 27.11.2015 15:04, Oleg Fayans wrote:
Hi Martin,

All your suggestions were taken into account. Both
patches are
updated. Thank you for your help!

On 11/26/2015 10:50 AM, Martin Basti wrote:
On 26.11.2015 10:04, Oleg Fayans wrote:
Hi Martin,

I agree to all your points but one. please,
see my
comment
below

On 11/25/2015 07:42 PM, Martin Basti wrote:
Hi,

0) Note
Please be aware of
https://fedorahosted.org/freeipa/ticket/5469
during
KRA testing

1)
Please do not use MIN and MAX_DOMAIN_LEVEL
constants,
this may
change
over time, use DOMAIN_LEVEL_0 and
DOMAIN_LEVEL_1 for
domain
level 0
and 1

2)
Why uninstall KRA then server, is not enough
just
uninstall
server
which
covers KRA uninstall?

+    def teardown_method(self, method):
+        for host in self.replicas:
+ host.run_command(self.kra_uninstall,
raiseonerr=False)
+ tasks.uninstall_master(host)


3)
Can be this function more generic? It should
allow
specify
host
where
KRA should be installed not just master

+    def test_kra_install_master(self):
+ self.master.run_command(self.kra_install)


4)

TestLevel0(Dummy):
Can be the test name more specific, something
like
TestReplicaPromotionLevel0


5)
please remove this, the patch is on review
and it
will be
pushed
sooner
than tests
+ @pytest.mark.xfail # Ticket N 5455

and as I mentioned in ticket #5455, I cannot
reproduce
it with
ipa-kra-install, so please provide steps to
reproduce if
you
insist
that
this still does not work as expected with KRA.

6) This is completely wrong, it removes
everything
that we
tried to
achieve with previous patches with domain
level in CI
Actually, being able to configure domain
level per
class
is WAY
more
convenient, than to always have to think which
domain
level is
appropriate for which particular test during
jenkins
job
configuration. In fact, I should have thought
about it
from the
very
beginning. For example, in
test_replica_promotion.py we
have on
class,
which intiates with domain level = 1, while
others -
with
domain
level
0. With config-based approach, we would have to
implement a
separate
step that raises domain level. Overall, I am
against
the
approach,
when you have to remember to set certain domain
level in
config
for
any particular test. The tests themselves
should be
aware of
the
domain level they need.
I do not say that we should not have something
that
overrides
settings
in from config in a particular test case, I say
your
patch is
doing it
wrong.

I agree it is useful to have param
domain_level in
install_master,
and
intall_topo methods, but is cannot be
MAX_DOMAIN_LEVEL by
default,
because with your current patch the
domain_level in
config is
not
used
at all, it will be always MAX_DOMAIN_LEVEL

For example I want to achieve this goal:
test_vault.py, this test suite can run on domain
level1
and on
domain
level0, so with one test we can test 2 domain
levels
just
with
putting
domain level into config file.

I agree that with extraordinary test like replica
promotion test
is, we
need something that allows override the config
file.

As I said bellow, domain_level default value
should be
None in
install_master and install_topo plugin. If
domain level
was
specified
use the specified one, if not (value is None)
use the
domain
level
from
config file.
Agreed :)

Martin
[PATCH] Enabled setting domain_level per class
derived from
TestIntegration

When I configure domain level 0 in yaml
config, how
is this
supposed to
get into install methods when you removed that
code?

- "--domain-level=%i" %
host.config.domain_level
+ "--domain-level=%i" % domain_level


You always use MAX_DOMAIN_LEVEL in this case or
whatever is
specified in
domain_level option.
I suggest to use domain_level=None, and when
it is
None use
'host.config.domain_level', if it is not None,
use
'domain_level'

With this we can specify domain level in config
file for
test
that
can
be used for both domain levels and you can
manually
specify
domain
level
for test that requires specific domain level.

Also this should go away

@classmethod
def install(cls, mh):
+        if hasattr(cls, "domain_level") and
cls.master:
+ cls.master.config.domain_level =
cls.domain_level
if cls.topology is None:
return

I do not see reason why test should override
configuration in
config in
this case.

Martin

On 25.11.2015 16:44, Oleg Fayans wrote:
Hi,

Here is the updated version of the patch (more
tests +
fixed the
issues of the first one) + patch 0017, that
implements the
necessary
changes in the background code, i. e. patch 16
does not
work
without
patch 17

On 11/18/2015 05:20 PM, Martin Basti wrote:
On 09.11.2015 15:09, Oleg Fayans wrote:
Hi guys,

Here are first two automated testcases from
this
(so far
incomplete)
testplan:
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan






Testplan review is highly appreciated




PATCH 16: NACK

1)
What is the reason to add an unused
parameter to
'domain_level' to
install_topo()?
Also it is good practise to add new option
as the
last
parameter.

2)
cab you in both tests specify a domain level
with
constant
instead of
number literal?

3)
both test call install_topo with custom
domain
level,
but it
cannot
work
because 1) (did you run the test?)

4)
How the test "TestLevel1" is supposed to
work?
Respectively why there is call of
install_topo()
that
installs
replica.
As this test just tests that
ipa-replica-prepare is
not
working
anymore,
is it worth to spend 20 minutes with
installing
replica and
then
just no
tot use it? IMO to install master in install
step is
enough.

Martin^2

./make-lint
************* Module ipatests.test_integration.base
ipatests/test_integration/base.py:66:
[E1101(no-member),
IntegrationTest.install] Class 'IntegrationTest'
has no
'domain_level'
member)
************* Module
ipatests.test_integration.test_replica_promotion
ipatests/test_integration/test_replica_promotion.py:16:


[E1101(no-member), Dummy.install] Class 'Dummy'
has no
'domain_level'
member)
ipatests/test_integration/test_replica_promotion.py:117:


[E1101(no-member),
TestCAInstall.test_ca_install_without_replica_file]
Module 'ipatests.test_integration.tasks' has no
'setup_replica'
member)


Is it so hard to run pylint before patch is posted
for
review?
Sorry, my bad. Fixed.



1)
Why is this change in the patch?
-    # Clean up the test directory
- host.run_command(['rm', '-rvf',
host.config.test_dir])
Otherwise 2 out of 8 tests fail with IOError at line
78 of
ipatests/test_integration/tasks.py

I do not understand yet how does this happen, but if
you
remove
ipatests folder once, it then fails to be created
again.

So this should be in separated patch and investigated
properly.
Agree. Removed
-


2)
is enough to have this check only in install_master,
install_topo can
pass None to install_master
+    if domain_level is None:
+        domain_level = master.config.domain_level
Done

3)
IMO replica-manage del should cleanup hosts entry, so
following
code
should not be needed.
+    if cleanhost:
+ kinit_admin(master)
+ master.run_command(["ipa", "host-del",
"--updatedns",
replica.hostname],
+ raiseonerr=False)
Well, in fact it does not. At least the
corresponding dns
record
stays
and causes the subsequent ipa-client-install to fail.
Probably
it's a
bug. On the other hand, if I promote an existing
client to
replica and
then delete this replica, then, I probably want the
host
record
(that
was created during client-install) to stay in the
system. So,
does not
look like a bug to me.
No you don't, because replica uninstallation also
removes the
client.

I tried it with ipa42, ipa-replica-manage del removes
host
entry,
and
DNS A records, only SSHFP records stay there (I'm not
sure
if it
is bug
or feature)

Also I received this message
"""
Failed to cleanup replica1.ipa.test DNS entries: no
matching
entry
found
You may need to manually remove them from the tree
"""
But, A record has been removed, so this is probably
false
positive and
it needs to have a ticket.
Agree, that was an issue with my setup.
Removed

4)
This variable is not used
+    kra_uninstall = ["ipa-kra-install",
"--uninstall", "-U"]
Removed

5)
Why do you need this
+    kra_install = ["ipa-kra-install", "-U", "-p",
config.dirman_password]
when you implemented tasks.install_kra that returns
the
exactly
the same
result?
Right. Removed

6)
PEP8
./ipatests/test_integration/tasks.py:928:1: E302
expected 2
blank
lines,
found 1
./ipatests/test_integration/tasks.py:934:1: E302
expected 2
blank
lines,
found 1
./ipatests/test_integration/tasks.py:939:1: E302
expected 2
blank
lines,
found 1
./ipatests/test_integration/tasks.py:943:1: E302
expected 2
blank
lines,
found 1
./ipatests/test_integration/tasks.py:950:80: E501
line too
long
(80 > 79
characters)

./ipatests/test_integration/test_replica_promotion.py:29:80:


E501
line
too long (85 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:64:80:


E501
line
too long (85 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:67:80:


E501
line
too long (88 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:93:80:


E501
line
too long (80 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:94:80:


E501
line
too long (83 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:118:80:


E501

line
too long (81 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:128:80:


E501

line
too long (80 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:129:80:


E501

line
too long (82 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:181:80:


E501

line
too long (80 > 79 characters)
Most of these complaints are unrelated to the current
patches.
It's better to create a separate patch addressing PEP8
errors.

I beg for your pardon, those PEP8 errors have been
introduced by
your
patches.
Fixed

7)
Why this must be stored in instance? IMO to have it
stored as
local
variable is perfect
TestKRAInstall, TestCAInstall
self.replica1_filename =
tasks.get_replica_filename(replica1)
self.replica2_filename =
tasks.get_replica_filename(replica2)
Agree. Fixed

This patch is missing something.
I am sorry, I forgot to revert my previous change. The
correct
patch is
attached

************* Module
ipatests.test_integration.test_replica_promotion
ipatests/test_integration/test_replica_promotion.py:15:
[E1123(unexpected-keyword-arg), Dummy.install] Unexpected
keyword
argument 'domain_level' in function call)
ipatests/test_integration/test_replica_promotion.py:15:
[E1101(no-member), Dummy.install] Class 'Dummy' has no
'domain_level'
member)
ipatests/test_integration/test_replica_promotion.py:19:
[E1101(no-member), Dummy.teardown_method] Module
'ipatests.test_integration.tasks' has no 'uninstall_replica'
member)
ipatests/test_integration/test_replica_promotion.py:67:
[E1101(no-member),
TestReplicaPromotionLevel0.test_backup_restore]
Module 'ipatests.test_integration.tasks' has no 'ipa_backup'
member)
ipatests/test_integration/test_replica_promotion.py:72:
[E1101(no-member),
TestReplicaPromotionLevel0.test_backup_restore]
Module 'ipatests.test_integration.tasks' has no 'ipa_restore'
member)
ipatests/test_integration/test_replica_promotion.py:120:
[E1123(unexpected-keyword-arg), TestCAInstall.install]
Unexpected
keyword argument 'domain_level' in function call)

Sorry I forgot to apply patch 17, my bad, I'm continuing with
review
LGTM, I haven't had time to test it, but if you are sure that
test is
working, we may push this.

Is this expected due the bug you mentioned?
_____
__________________________________________________________________________



TestReplicaPromotionLevel0.test_kra_install_master
________________________________________________________________________________





self =
<ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0



object at 0x7f5071a59e50>

        def test_kra_install_master(self):
            result1 = tasks.install_kra(self.master,
raiseonerr=False)
          assert result1.returncode == 0, result1.stderr_text
E       AssertionError: Usage: ipa-kra-install [options]
[replica_file]
E
E         ipa-kra-install: error: Replica file
/root/ipatests/replica-info.gpg does not exist
E         The ipa-kra-install command failed. See
/var/log/ipaserver-kra-install.log for more information
E
E       assert 2 == 0
E        +  where 2 = <pytest_multihost.transport.SSHCommand
object at
0x7f5071adbd50>.returncode

IMO the test needs fix, KRA on replica file needs KRA related
certificates in replica file

[ipa.ipatests.test_integration.host.Host.replica2.ParamikoTransport]

RUN
['ipa-kra-install', '-U', '-p', 'Secret123',
'/root/ipatests/replica-info.gpg']
[ipa.ipatests.test_integration.host.Host.replica2.cmd27] RUN
['ipa-kra-install', '-U', '-p', 'Secret123',
'/root/ipatests/replica-info.gpg']
[ipa.ipatests.test_integration.host.Host.replica2.cmd27] Missing
KRA
certificates, please create a new replica file.
[ipa.ipatests.test_integration.host.Host.replica2.cmd27] The
ipa-kra-install command failed. See
/var/log/ipaserver-kra-install.log
for more information
[ipa.ipatests.test_integration.host.Host.replica2.cmd27] Exit
code: 1
FAILED
traceback

self =
<ipatests.test_integration.test_replica_promotion.TestKRAInstall
object at 0x7f660bc1a590>

I just read the code.

PATCH 16:
0)
PEP8
./ipatests/test_integration/test_replica_promotion.py:24:14: E111
indentation is not a multiple of four
./ipatests/test_integration/test_replica_promotion.py:24:14: E113
unexpected indentation
./ipatests/test_integration/test_replica_promotion.py:148:80: E501
line too long (80 > 79 characters)
./ipatests/test_integration/test_replica_promotion.py:150:80: E501
line too long (81 > 79 characters)

1)
workaround is not workaround, because the host entry is removed
anyway, the error is raised from POST callback, please remove it
+             # Workaround for 5627
+            if "host not found" in result.stderr_text:
+                self.master.run_command(["ipa",
+                                         "host-del",
+ host.hostname],
raiseonerr=False)
sorry, I was wrong with this, check is in pre_callback, but please
remove it anyway, I will send patch to fix it ASAP
Done
I realized that the fix I'm working on is for 4.4 only, so for 4.3 add
this as separated patch.
Done, patch 0027

2)
Please name it better, for example "replica" instead of "i"
+        for i in self.replicas:
+            tasks.install_replica(master, i, setup_ca=False,
+                                  setup_dns=True)
Done

3)
Please use constant for domain level (multiple times)
+ result1 = tasks.install_ca(replica1, domain_level=1,
raiseonerr=False)

+        tasks.install_ca(replica1, domain_level=0)
+        result2 = tasks.install_ca(replica2, domain_level=0,
raiseonerr=False)
... more times
Done

4)
This link does not exists, only connect is deprecated not
ipa-replica-manage at all
+    def test_replica_manage_commands(self):
+        """
+        TestCase:
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+ #Test_case:_ipa-replica-manage_is_deprecated_in_domain_level_1
+        """
Fixed

5)
Missing testcases:

Test case: Unprivileged users are not allowed to enroll and promote
clients
Test case: Replica created using old workflow is functional after
domain upgrade
Test case: ipa-csreplica-manage connect is deprecated in domain level 1
Test case: Replica can be installed using one command
Test case: Prohibit ipa server uninstallation from disconnecting
topology segment

They are on the way, not fully ready yet

PATCH 24:

1)
why there is this change, how it is related to this patch?:
   def apply_common_fixes(host):
+    prepare_host(host)
       fix_etc_hosts(host)
       fix_hostname(host)
-    prepare_host(host)
Good catch! That was one of my attempts to address the issue that was
successfully resolved in patch 0025. Will remove it once we agree on the
rest of the changes
Removed

2)
Why is there this change, how it is related to this patch?:
   def replica_prepare(master, replica):
-    apply_common_fixes(replica)
       fix_apache_semaphores(replica)
...
def install_replica(master, replica, setup_ca=True, setup_dns=False,
...
+    apply_common_fixes(replica)
Just to make this call independent from domain level (at domain_level 1
replica_prepare never gets called)
It should be in separate commit, because it is not related to adding
domain_level in class functionality
Done. Patch 0026


3)
why is there this change, how it is related to this patch?:
-
+        args.extend(['-n', replica.domain.name,
+                     '-r', replica.domain.realm])
At least -r is a required parameter. -n was added for further
robustness. Can be safely removed, though
It should be in separate commit, as this is not related to domain levels
Done. Patch 0026

4)
why there force, how is this change related to this patch (domain
levels)?
                           '-w', client.config.admin_password,
-                        '--server', master.hostname]
+                        '--server', master.hostname,
+                        '--force']
                          + list(extra_args))
client refuses to install unless everything is super clear in the dns
setup (including reverse zone). Otherwise the installer fails and
informs you that you may use '--force' at your own risk. I can rerun the
tests without this option to provide you with the exact output, if you
like.
It should be in separated commit, because it is not related to domain
levels
I've run the tests without this option again at it passed. Must have
been some temporary issue. Removed this change.

Otherwise domain level related changes LGTM

PATCH 25

LGTM

Martin^2


1)
this method is unused please remove it

    def test_kra_install_master(self):

2)
Why are these there? I do not see any usage

from env_config import get_global_config
config = get_global_config()

3) nitpick
+    num_clients = 0
this is set by default

otherwise LGTM

Results of testing tomorrow.

Martin^2


I applied all patches including workarounds, but test failed.

ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0

[ipa.ipatests.test_integration.host.Host.replica1.cmd51] RUN ['ipa-replica-install', '-U', '-p', 'Secret123', '-w', 'Secret123', '--setup-ca', '--ip-address', '192.168.144.102', '/root/ipatests/replica-info.gpg'] [ipa.ipatests.test_integration.host.Host.replica1.cmd51] The host replica1.ipa.test already exists on the master server. [ipa.ipatests.test_integration.host.Host.replica1.cmd51] You should remove it before proceeding: [ipa.ipatests.test_integration.host.Host.replica1.cmd51] % ipa host-del replica1.ipa.test [ipa.ipatests.test_integration.host.Host.replica1.cmd51] ipa.ipapython.install.cli.install_tool(Replica): ERROR The ipa-replica-install command failed. See /var/log/ipareplica-install.log for more information
[ipa.ipatests.test_integration.host.Host.replica1.cmd51] Exit code: 3
FAILED

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to